shortcuts: Cache Keycode to DomKey string conversions

Improves performance of looking up the Domkey string from a
Keycode. Previously, it would perform an inefficient O(N) lookup
for every keycode. This change adds a cache so that lookups are
amortized down closer to constant lookup time.

Since the cache is IME-dependent, anytime the IME changes the cache
is cleared and will have to be rebuilt.

This also has the side effect of reducing the spammy logs from
DomCode -> DomKey lookups.

Test: ash_unittests
Bug: b:271095602 b:216049298
Change-Id: I19d90ce66c76c11da78019e4f66a65037706be72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4299984
Reviewed-by: David Padlipsky <dpad@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1121820}

ash/public/cpp/BUILD.gn

@@ -12,6 +12,8 @@ component("cpp") {
   sources = [
     "accelerator_configuration.cc",
     "accelerator_configuration.h",
+    "accelerator_keycode_lookup_cache.cc",
+    "accelerator_keycode_lookup_cache.h",
     "accelerators.cc",
     "accelerators.h",
     "accelerators_util.cc",
@@ -449,6 +451,7 @@ component("cpp") {
     "//components/session_manager:base",
     "//components/user_manager",
     "//components/version_info:channel",
+    "//ui/base/ime/ash",
     "//ui/base/ime/ash:ime_types",
     "//ui/gfx",
   ]
@@ -459,6 +462,7 @@ component("cpp") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "accelerator_keycode_lookup_cache_unittest.cc",
     "accelerators_util_unittest.cc",
     "ambient/ambient_metrics_unittest.cc",
     "ambient/ambient_prefs_unittest.cc",
@@ -486,6 +490,7 @@ source_set("unit_tests") {
   deps = [
     ":cpp",
     ":test_support",
+    "//ash:test_support",
     "//base",
     "//base/test:test_support",
     "//chromeos/ui/vector_icons",
@@ -495,6 +500,7 @@ source_set("unit_tests") {
     "//skia",
     "//testing/gtest",
     "//ui/aura:test_support",
+    "//ui/base:test_support",
     "//ui/chromeos/styles:cros_styles_views",
     "//ui/chromeos/styles:cros_tokens_color_mappings",
     "//ui/compositor_extra",

ash/public/cpp/accelerator_keycode_lookup_cache.cc

@@ -0,0 +1,65 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/public/cpp/accelerator_keycode_lookup_cache.h"
+
+#include <string>
+
+#include "base/no_destructor.h"
+#include "ui/base/ime/ash/input_method_manager.h"
+#include "ui/events/keycodes/keyboard_codes_posix.h"
+
+namespace ash {
+namespace {
+
+AcceleratorKeycodeLookupCache* g_instance = nullptr;
+
+}  // namespace
+
+AcceleratorKeycodeLookupCache* AcceleratorKeycodeLookupCache::Get() {
+  return g_instance;
+}
+
+AcceleratorKeycodeLookupCache::AcceleratorKeycodeLookupCache() {
+  CHECK(!g_instance);
+  g_instance = this;
+
+  DCHECK(input_method::InputMethodManager::Get());
+  input_method::InputMethodManager::Get()->AddObserver(this);
+}
+
+AcceleratorKeycodeLookupCache::~AcceleratorKeycodeLookupCache() {
+  CHECK(input_method::InputMethodManager::Get());
+  input_method::InputMethodManager::Get()->RemoveObserver(this);
+
+  CHECK_EQ(g_instance, this);
+  g_instance = nullptr;
+}
+
+void AcceleratorKeycodeLookupCache::InputMethodChanged(
+    input_method::InputMethodManager* manager,
+    Profile* profile,
+    bool show_message) {
+  key_code_to_string16_cache_.clear();
+}
+
+absl::optional<std::u16string> AcceleratorKeycodeLookupCache::Find(
+    ui::KeyboardCode key_code) {
+  const auto& found_iter = key_code_to_string16_cache_.find(key_code);
+  if (found_iter == key_code_to_string16_cache_.end()) {
+    return absl::nullopt;
+  }
+  return found_iter->second;
+}
+
+void AcceleratorKeycodeLookupCache::InsertOrAssign(ui::KeyboardCode key_code,
+                                                   std::u16string description) {
+  key_code_to_string16_cache_.insert_or_assign(key_code, description);
+}
+
+void AcceleratorKeycodeLookupCache::Clear() {
+  key_code_to_string16_cache_.clear();
+}
+
+}  // namespace ash

ash/public/cpp/accelerator_keycode_lookup_cache.h

@@ -0,0 +1,55 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_PUBLIC_CPP_ACCELERATOR_KEYCODE_LOOKUP_CACHE_H_
+#define ASH_PUBLIC_CPP_ACCELERATOR_KEYCODE_LOOKUP_CACHE_H_
+
+#include <map>
+#include <string>
+
+#include "ash/public/cpp/ash_public_export.h"
+#include "ui/base/ime/ash/input_method_manager.h"
+#include "ui/events/keycodes/keyboard_codes_posix.h"
+
+namespace ash {
+
+// Thin class that observe IME change events and updates the static cache for
+// more efficient lookups for `KeycodeToKeyString()`.
+class ASH_PUBLIC_EXPORT AcceleratorKeycodeLookupCache
+    : public input_method::InputMethodManager::Observer {
+ public:
+  // Get the singleton instance.
+  static AcceleratorKeycodeLookupCache* Get();
+
+  AcceleratorKeycodeLookupCache();
+  AcceleratorKeycodeLookupCache(const AcceleratorKeycodeLookupCache&) = delete;
+  AcceleratorKeycodeLookupCache& operator=(
+      const AcceleratorKeycodeLookupCache&) = delete;
+  ~AcceleratorKeycodeLookupCache() override;
+
+  // InputMethodManager::Observer
+  // The cache is invalid when the IME changes, so this observer notifies us
+  // when to clear the cache.
+  void InputMethodChanged(input_method::InputMethodManager* manager,
+                          Profile* profile,
+                          bool show_message) override;
+
+  absl::optional<std::u16string> Find(ui::KeyboardCode key_code);
+
+  void InsertOrAssign(ui::KeyboardCode key_code, std::u16string description);
+
+  void Clear();
+
+ private:
+  friend class AcceleratorKeycodeLookupCacheTest;
+
+  // A cache of Keycode to string16 so that we don't have to perform a
+  // reverse lookup for the DomKey for every accelerator.
+  // On IME changes, this cache resets.
+  std::map<ui::KeyboardCode, std::u16string> key_code_to_string16_cache_;
+};
+
+}  // namespace ash
+
+#endif  // ASH_PUBLIC_CPP_ACCELERATOR_KEYCODE_LOOKUP_CACHE_H_

ash/public/cpp/accelerator_keycode_lookup_cache_unittest.cc

@@ -0,0 +1,88 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/public/cpp/accelerator_keycode_lookup_cache.h"
+
+#include <memory>
+#include <string>
+
+#include "ui/base/ime/ash/input_method_manager.h"
+#include "ui/base/ime/ash/mock_input_method_manager.h"
+#include "ui/events/keycodes/keyboard_codes_posix.h"
+#include "ui/events/ozone/layout/keyboard_layout_engine_manager.h"
+#include "ui/events/ozone/layout/stub/stub_keyboard_layout_engine.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ash {
+
+class AcceleratorKeycodeLookupCacheTest : public testing::Test {
+ public:
+  class TestInputMethodManager : public input_method::MockInputMethodManager {
+   public:
+    void AddObserver(
+        input_method::InputMethodManager::Observer* observer) override {
+      observers_.AddObserver(observer);
+    }
+
+    void RemoveObserver(
+        input_method::InputMethodManager::Observer* observer) override {
+      observers_.RemoveObserver(observer);
+    }
+
+    // Calls all observers with Observer::InputMethodChanged
+    void NotifyInputMethodChanged() {
+      for (auto& observer : observers_) {
+        observer.InputMethodChanged(
+            /*manager=*/this, /*profile=*/nullptr, /*show_message=*/false);
+      }
+    }
+
+    base::ObserverList<InputMethodManager::Observer>::Unchecked observers_;
+  };
+
+  AcceleratorKeycodeLookupCacheTest() {
+    input_method_manager_ = new TestInputMethodManager();
+    input_method::InputMethodManager::Initialize(input_method_manager_);
+
+    layout_engine_ = std::make_unique<ui::StubKeyboardLayoutEngine>();
+    ui::KeyboardLayoutEngineManager::ResetKeyboardLayoutEngine();
+    ui::KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
+        layout_engine_.get());
+
+    lookup_cache_ = std::make_unique<AcceleratorKeycodeLookupCache>();
+  }
+
+  ~AcceleratorKeycodeLookupCacheTest() override = default;
+
+ protected:
+  std::map<ui::KeyboardCode, std::u16string> cache() {
+    return lookup_cache_->key_code_to_string16_cache_;
+  }
+
+  std::unique_ptr<AcceleratorKeycodeLookupCache> lookup_cache_;
+  std::unique_ptr<ui::StubKeyboardLayoutEngine> layout_engine_;
+  // Test global singleton. Delete is handled by InputMethodManager::Shutdown().
+  base::raw_ptr<TestInputMethodManager> input_method_manager_;
+};
+
+TEST_F(AcceleratorKeycodeLookupCacheTest, ImeChanged) {
+  const std::u16string expected = u"a";
+
+  EXPECT_TRUE(cache().empty());
+  lookup_cache_->InsertOrAssign(ui::KeyboardCode::VKEY_A, expected);
+  // Expect the cache to be populated.
+  absl::optional<std::u16string> found_key_string =
+      lookup_cache_->Find(ui::KeyboardCode::VKEY_A);
+  EXPECT_TRUE(found_key_string.has_value());
+  EXPECT_EQ(expected, found_key_string.value());
+
+  // Trigger IME change event, expect the cache to be cleared.
+  input_method_manager_->NotifyInputMethodChanged();
+  EXPECT_TRUE(cache().empty());
+  found_key_string = lookup_cache_->Find(ui::KeyboardCode::VKEY_A);
+  EXPECT_FALSE(found_key_string.has_value());
+}
+
+}  // namespace ash

ash/public/cpp/accelerators_util.cc

@@ -6,8 +6,10 @@
 
 #include <string>
 
+#include "ash/public/cpp/accelerator_keycode_lookup_cache.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
+#include "ui/base/ime/ash/input_method_manager.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/dom/dom_codes_array.h"
@@ -71,6 +73,8 @@ std::u16string KeycodeToKeyString(ui::KeyboardCode key_code,
   // normally in the loop below. For the positional keys, the |DomCode| is
   // then mapped to the |DomKey| in the current layout which represents the
   // glyph/character that appears on the key (and usually when typed).
+
+  // Positional keys are direct lookups, no need to store in the cache.
   if (remap_positional_key &&
       ::features::IsImprovedKeyboardShortcutsEnabled()) {
     ui::DomCode dom_code =
@@ -91,16 +95,38 @@ std::u16string KeycodeToKeyString(ui::KeyboardCode key_code,
     }
   }
 
+  const absl::optional<std::u16string> cached_key_string =
+      AcceleratorKeycodeLookupCache::Get()->Find(key_code);
+  // Cache hit, return immediately.
+  if (cached_key_string.has_value()) {
+    return std::move(cached_key_string).value();
+  }
+
+  // Cache miss, get the key string and store it.
   for (const auto& dom_code : ui::kDomCodesArray) {
     if (!layout_engine->Lookup(dom_code, /*event_flags=*/ui::EF_NONE, &dom_key,
                                &key_code_to_compare)) {
       continue;
     }
-    if (key_code_to_compare != key_code || !dom_key.IsValid() ||
-        dom_key.IsDeadKey()) {
+
+    // Even though this isn't what we're looking for, we should still populate
+    // the cache as we're iterating through the DomCode array.
+    if (key_code_to_compare != key_code) {
+      AcceleratorKeycodeLookupCache::Get()->InsertOrAssign(
+          key_code_to_compare,
+          base::UTF8ToUTF16(ui::KeycodeConverter::DomKeyToKeyString(dom_key)));
       continue;
     }
-    return base::UTF8ToUTF16(ui::KeycodeConverter::DomKeyToKeyString(dom_key));
+
+    if (!dom_key.IsValid() || dom_key.IsDeadKey()) {
+      continue;
+    }
+
+    // Found the correct lookup, cache and return the string.
+    const std::u16string key_string =
+        base::UTF8ToUTF16(ui::KeycodeConverter::DomKeyToKeyString(dom_key));
+    AcceleratorKeycodeLookupCache::Get()->InsertOrAssign(key_code, key_string);
+    return key_string;
   }
   return std::u16string();
 }

ash/public/cpp/accelerators_util_unittest.cc

@@ -7,7 +7,11 @@
 #include <memory>
 #include <string>
 
+#include "ash/public/cpp/accelerator_keycode_lookup_cache.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
 #include "base/strings/utf_string_conversions.h"
+#include "ui/base/ime/ash/mock_input_method_manager.h"
 #include "ui/events/keycodes/dom/dom_codes_array.h"
 #include "ui/events/keycodes/dom/dom_key.h"
 #include "ui/events/keycodes/keyboard_codes_posix.h"
@@ -18,7 +22,7 @@
 
 namespace ash {
 
-class AcceleratorsUtilTest : public testing::Test {
+class AcceleratorsUtilTest : public AshTestBase {
  public:
   AcceleratorsUtilTest() {
     layout_engine_ = std::make_unique<ui::StubKeyboardLayoutEngine>();
@@ -35,7 +39,16 @@ class AcceleratorsUtilTest : public testing::Test {
 
 TEST_F(AcceleratorsUtilTest, BasicDomCode) {
   const std::u16string expected = u"a";
+
+  absl::optional<std::u16string> found_key_string =
+      AcceleratorKeycodeLookupCache::Get()->Find(ui::KeyboardCode::VKEY_A);
+  EXPECT_FALSE(found_key_string.has_value());
   EXPECT_EQ(expected, KeycodeToKeyString(ui::KeyboardCode::VKEY_A));
+  // Expect the cache to be populated.
+  found_key_string =
+      AcceleratorKeycodeLookupCache::Get()->Find(ui::KeyboardCode::VKEY_A);
+  EXPECT_TRUE(found_key_string.has_value());
+  EXPECT_EQ(expected, found_key_string.value());
 }
 
 TEST_F(AcceleratorsUtilTest, PositionalKeyCode) {
@@ -64,7 +77,16 @@ TEST_F(AcceleratorsUtilTest, PositionalKeyCode) {
 
 TEST_F(AcceleratorsUtilTest, NonAlphanumericKey) {
   const std::u16string expected = u"Meta";
+  absl::optional<std::u16string> found_key_string =
+      AcceleratorKeycodeLookupCache::Get()->Find(
+          ui::KeyboardCode::VKEY_COMMAND);
+  EXPECT_FALSE(found_key_string.has_value());
   EXPECT_EQ(expected, KeycodeToKeyString(ui::KeyboardCode::VKEY_COMMAND));
+
+  found_key_string = AcceleratorKeycodeLookupCache::Get()->Find(
+      ui::KeyboardCode::VKEY_COMMAND);
+  EXPECT_TRUE(found_key_string.has_value());
+  EXPECT_EQ(expected, found_key_string.value());
 }
 
 }  // namespace ash

ash/shell.cc

@@ -92,6 +92,7 @@
 #include "ash/multi_device_setup/multi_device_notification_presenter.h"
 #include "ash/policy/policy_recommendation_restorer.h"
 #include "ash/projector/projector_controller_impl.h"
+#include "ash/public/cpp/accelerator_keycode_lookup_cache.h"
 #include "ash/public/cpp/ash_prefs.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/nearby_share_delegate.h"
@@ -1326,6 +1327,9 @@ void Shell::Init(
   cursor_manager_->SetDisplay(
       display::Screen::GetScreen()->GetPrimaryDisplay());
 
+  // Must be initialized after InputMethodManager.
+  accelerator_keycode_lookup_cache_ =
+      std::make_unique<AcceleratorKeycodeLookupCache>();
   ash_accelerator_configuration_ =
       std::make_unique<AshAcceleratorConfiguration>();
   ash_accelerator_configuration_->Initialize();

ash/shell.h

@@ -89,6 +89,7 @@ class WindowModalityController;
 namespace ash {
 
 class AcceleratorControllerImpl;
+class AcceleratorKeycodeLookupCache;
 class AcceleratorTracker;
 class AccessibilityControllerImpl;
 class AccessibilityDelegate;
@@ -895,6 +896,8 @@ class ASH_EXPORT Shell : public SessionObserver,
 
   std::unique_ptr<AshAcceleratorConfiguration> ash_accelerator_configuration_;
   std::unique_ptr<AcceleratorControllerImpl> accelerator_controller_;
+  std::unique_ptr<AcceleratorKeycodeLookupCache>
+      accelerator_keycode_lookup_cache_;
   std::unique_ptr<AccessibilityControllerImpl> accessibility_controller_;
   std::unique_ptr<AccessibilityDelegate> accessibility_delegate_;
   std::unique_ptr<AccessibilityFocusRingControllerImpl>