Accelerators: Implement positional accelerator map

- Pull a thin wrapper over std::map<Accelerator, V> out to a separate
  class
- Allow the map to enable and disable positional lookup on Chrome OS
- Add functions to map the positional keys back and forth
- In this CL positional lookup is still disabled so this has
  no effect on behavior
- Add tests for AcceleratorMap for positional use cases

Bug: 1174326
Test: ui_base_unittests --gtest_filter=AcceleratorManagerTest.*
Change-Id: I310d67792b7f5259c6962da598010f2e98b406c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2792525
Commit-Queue: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#870862}

ash/accelerators/accelerator_controller_impl.cc

@@ -1720,8 +1720,8 @@ void AcceleratorControllerImpl::UnregisterAll(ui::AcceleratorTarget* target) {
 
 bool AcceleratorControllerImpl::IsActionForAcceleratorEnabled(
     const ui::Accelerator& accelerator) const {
-  auto it = accelerators_.find(accelerator);
-  return it != accelerators_.end() && CanPerformAction(it->second, accelerator);
+  const AcceleratorAction* action_ptr = accelerators_.Find(accelerator);
+  return action_ptr && CanPerformAction(*action_ptr, accelerator);
 }
 
 bool AcceleratorControllerImpl::Process(const ui::Accelerator& accelerator) {
@@ -1747,12 +1747,9 @@ bool AcceleratorControllerImpl::OnMenuAccelerator(
     const ui::Accelerator& accelerator) {
   accelerator_history()->StoreCurrentAccelerator(accelerator);
 
-  auto itr = accelerators_.find(accelerator);
-  if (itr == accelerators_.end())
-    return false;  // Menu shouldn't be closed for an invalid accelerator.
-
-  AcceleratorAction action = itr->second;
-  return !base::Contains(actions_keeping_menu_open_, action);
+  // Menu shouldn't be closed for an invalid accelerator.
+  AcceleratorAction* action_ptr = accelerators_.Find(accelerator);
+  return action_ptr && !base::Contains(actions_keeping_menu_open_, *action_ptr);
 }
 
 bool AcceleratorControllerImpl::IsRegistered(
@@ -1766,20 +1763,14 @@ AcceleratorHistoryImpl* AcceleratorControllerImpl::GetAcceleratorHistory() {
 
 bool AcceleratorControllerImpl::IsPreferred(
     const ui::Accelerator& accelerator) const {
-  auto iter = accelerators_.find(accelerator);
-  if (iter == accelerators_.end())
-    return false;  // not an accelerator.
-
-  return base::Contains(preferred_actions_, iter->second);
+  const AcceleratorAction* action_ptr = accelerators_.Find(accelerator);
+  return action_ptr && base::Contains(preferred_actions_, *action_ptr);
 }
 
 bool AcceleratorControllerImpl::IsReserved(
     const ui::Accelerator& accelerator) const {
-  auto iter = accelerators_.find(accelerator);
-  if (iter == accelerators_.end())
-    return false;  // not an accelerator.
-
-  return base::Contains(reserved_actions_, iter->second);
+  const AcceleratorAction* action_ptr = accelerators_.Find(accelerator);
+  return action_ptr && base::Contains(reserved_actions_, *action_ptr);
 }
 
 AcceleratorControllerImpl::AcceleratorProcessingRestriction
@@ -1792,9 +1783,7 @@ AcceleratorControllerImpl::GetCurrentAcceleratorRestriction() {
 
 bool AcceleratorControllerImpl::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
-  auto it = accelerators_.find(accelerator);
-  DCHECK(it != accelerators_.end());
-  AcceleratorAction action = it->second;
+  AcceleratorAction action = accelerators_.Get(accelerator);
   if (!CanPerformAction(action, accelerator))
     return false;
 
@@ -1891,7 +1880,8 @@ void AcceleratorControllerImpl::RegisterAccelerators(
         CreateAccelerator(accelerators[i].keycode, accelerators[i].modifiers,
                           accelerators[i].trigger_on_press);
     ui_accelerators.push_back(accelerator);
-    accelerators_.insert(std::make_pair(accelerator, accelerators[i].action));
+    accelerators_.InsertNew(
+        std::make_pair(accelerator, accelerators[i].action));
   }
   Register(ui_accelerators, this);
 }
@@ -1910,7 +1900,8 @@ void AcceleratorControllerImpl::RegisterDeprecatedAccelerators() {
                           accelerator_data.trigger_on_press);
 
     ui_accelerators.push_back(deprecated_accelerator);
-    accelerators_[deprecated_accelerator] = accelerator_data.action;
+    accelerators_.InsertNew(
+        std::make_pair(deprecated_accelerator, accelerator_data.action));
     deprecated_accelerators_.insert(deprecated_accelerator);
   }
   Register(ui_accelerators, this);

ash/accelerators/accelerator_controller_impl.h

@@ -22,6 +22,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "ui/base/accelerators/accelerator.h"
+#include "ui/base/accelerators/accelerator_map.h"
 
 namespace ui {
 class AcceleratorManager;
@@ -230,7 +231,7 @@ class ASH_EXPORT AcceleratorControllerImpl : public ui::AcceleratorTarget,
 
  private:
   // A map for looking up actions from accelerators.
-  using AcceleratorActionMap = std::map<ui::Accelerator, AcceleratorAction>;
+  using AcceleratorActionMap = ui::AcceleratorMap<AcceleratorAction>;
 
   // Initializes the accelerators this class handles as a target.
   void Init();

ui/base/BUILD.gn

@@ -329,6 +329,7 @@ component("base") {
       "accelerators/accelerator.h",
       "accelerators/accelerator_manager.cc",
       "accelerators/accelerator_manager.h",
+      "accelerators/accelerator_map.h",
       "accelerators/media_keys_listener.cc",
       "accelerators/media_keys_listener.h",
       "accelerators/media_keys_util.cc",
@@ -462,6 +463,7 @@ component("base") {
     # iOS does not use Chromium-specific code for event handling.
     public_deps += [
       "//ui/base/clipboard:file_info",
+      "//ui/events:dom_keycode_converter",
       "//ui/events:events_base",
       "//ui/events/platform",
     ]
@@ -966,6 +968,7 @@ test("ui_base_unittests") {
   } else {  # !is_ios
     sources += [
       "accelerators/accelerator_manager_unittest.cc",
+      "accelerators/accelerator_map_unittest.cc",
       "accelerators/accelerator_unittest.cc",
       "accelerators/menu_label_accelerator_util_unittest.cc",
       "models/dialog_model_unittest.cc",

ui/base/accelerators/accelerator.cc

@@ -31,7 +31,7 @@
 #include "ui/events/keycodes/keyboard_code_conversion.h"
 #endif
 
-#if BUILDFLAG(IS_CHROMEOS_ASH)
+#if defined(OS_CHROMEOS)
 #include "ui/base/ui_base_features.h"
 #endif
 
@@ -108,6 +108,20 @@ Accelerator::Accelerator(KeyboardCode key_code,
       time_stamp_(time_stamp),
       interrupted_by_mouse_event_(false) {}
 
+#if defined(OS_CHROMEOS)
+Accelerator::Accelerator(KeyboardCode key_code,
+                         DomCode code,
+                         int modifiers,
+                         KeyState key_state,
+                         base::TimeTicks time_stamp)
+    : key_code_(key_code),
+      code_(code),
+      key_state_(key_state),
+      modifiers_(modifiers & kInterestingFlagsMask),
+      time_stamp_(time_stamp),
+      interrupted_by_mouse_event_(false) {}
+#endif
+
 Accelerator::Accelerator(const KeyEvent& key_event)
     : key_code_(key_event.key_code()),
       key_state_(key_event.type() == ET_KEY_PRESSED ? KeyState::PRESSED
@@ -117,8 +131,15 @@ Accelerator::Accelerator(const KeyEvent& key_event)
       time_stamp_(key_event.time_stamp()),
       interrupted_by_mouse_event_(false),
       source_device_id_(key_event.source_device_id()) {
+#if defined(OS_CHROMEOS)
+  if (features::IsImprovedKeyboardShortcutsEnabled()) {
+    code_ = key_event.code();
+  }
+#endif
+
 #if BUILDFLAG(IS_CHROMEOS_ASH)
   if (features::IsNewShortcutMappingEnabled()) {
+    DCHECK(!features::IsImprovedKeyboardShortcutsEnabled());
     DomKey dom_key = key_event.GetDomKey();
     if (!dom_key.IsCharacter())
       return;
@@ -154,7 +175,11 @@ KeyEvent Accelerator::ToKeyEvent() const {
   return KeyEvent(key_state() == Accelerator::KeyState::PRESSED
                       ? ET_KEY_PRESSED
                       : ET_KEY_RELEASED,
-                  key_code(), modifiers(), time_stamp());
+                  key_code(),
+#if defined(OS_CHROMEOS)
+                  code(),
+#endif
+                  modifiers(), time_stamp());
 }
 
 bool Accelerator::operator<(const Accelerator& rhs) const {

ui/base/accelerators/accelerator.h

@@ -21,6 +21,10 @@
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 
+#if defined(OS_CHROMEOS)
+#include "ui/events/keycodes/dom/dom_code.h"
+#endif
+
 namespace ui {
 
 class KeyEvent;
@@ -46,6 +50,26 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator {
               int modifiers,
               KeyState key_state = KeyState::PRESSED,
               base::TimeTicks time_stamp = base::TimeTicks());
+
+#if defined(OS_CHROMEOS)
+  // Additional constructor that takes a |DomCode| in order to implement
+  // layout independent fixed position shortcuts. This is only used for
+  // shortcuts in Chrome OS. One such example is Alt ']'. In the US layout ']'
+  // is VKEY_OEM_6, in the DE layout it is VKEY_OEM_PLUS. However the key in
+  // that position is always DomCode::BRACKET_RIGHT regardless of what the key
+  // generates when pressed. When the DE layout is used and the accelerator
+  // is created with { VKEY_OEM_PLUS, DomCode::BRACKET_RIGHT } the custom
+  // accelerator map will map BRACKET_RIGHT to VKEY_OEM_6 as if in the US
+  // layout in order to lookup the accelerator.
+  //
+  // See accelerator_map.h for more information.
+  Accelerator(KeyboardCode key_code,
+              DomCode code,
+              int modifiers,
+              KeyState key_state = KeyState::PRESSED,
+              base::TimeTicks time_stamp = base::TimeTicks());
+#endif
+
   explicit Accelerator(const KeyEvent& key_event);
   Accelerator(const Accelerator& accelerator);
   Accelerator& operator=(const Accelerator& accelerator);
@@ -67,6 +91,10 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator {
 
   KeyboardCode key_code() const { return key_code_; }
 
+#if defined(OS_CHROMEOS)
+  DomCode code() const { return code_; }
+#endif
+
   // Sets the key state that triggers the accelerator. Default is PRESSED.
   void set_key_state(KeyState state) { key_state_ = state; }
   KeyState key_state() const { return key_state_; }
@@ -107,6 +135,11 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator {
   // The keycode (VK_...).
   KeyboardCode key_code_;
 
+#if defined(OS_CHROMEOS)
+  // The DomCode representing a key's physical position.
+  DomCode code_ = DomCode::NONE;
+#endif
+
   KeyState key_state_;
 
   // The state of the Shift/Ctrl/Alt keys. This corresponds to Event::flags().

ui/base/accelerators/accelerator_manager.cc

@@ -20,25 +20,24 @@ void AcceleratorManager::Register(
   DCHECK(target);
 
   for (const ui::Accelerator& accelerator : accelerators) {
-    accelerators_[accelerator].RegisterWithPriority(target, priority);
+    accelerators_.GetOrInsertDefault(accelerator)
+        .RegisterWithPriority(target, priority);
   }
 }
 
 void AcceleratorManager::Unregister(const Accelerator& accelerator,
                                     AcceleratorTarget* target) {
   DCHECK(target);
-  auto map_iter = accelerators_.find(accelerator);
-  DCHECK(map_iter != accelerators_.end())
-      << "Unregistering non-existing accelerator";
+  AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator);
+  DCHECK(target_info) << "Unregistering non-existing accelerator";
 
-  AcceleratorTargetInfo& target_info = map_iter->second;
-  const bool was_registered = target_info.Unregister(target);
+  const bool was_registered = target_info->Unregister(target);
   DCHECK(was_registered) << "Unregistering accelerator for wrong target";
 
   // If the last target for the accelerator is removed, then erase the
   // entry from the map.
-  if (!target_info.HasTargets())
-    accelerators_.erase(map_iter);
+  if (!target_info->HasTargets())
+    accelerators_.Erase(accelerator);
 }
 
 void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) {
@@ -49,7 +48,9 @@ void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) {
     // Unregister the target and remove the entry if it was the last target.
     const bool was_registered = target_info.Unregister(target);
     if (was_registered && !target_info.HasTargets()) {
-      map_iter = accelerators_.erase(map_iter);
+      Accelerator key_to_remove = map_iter->first;
+      ++map_iter;
+      accelerators_.Erase(key_to_remove);
       continue;
     }
 
@@ -59,35 +60,31 @@ void AcceleratorManager::UnregisterAll(AcceleratorTarget* target) {
 }
 
 bool AcceleratorManager::IsRegistered(const Accelerator& accelerator) const {
-  auto map_iter = accelerators_.find(accelerator);
+  const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator);
 
   // If the accelerator is in the map, the target list should not be empty.
-  DCHECK(map_iter == accelerators_.end() || map_iter->second.HasTargets());
-  return map_iter != accelerators_.end();
+  DCHECK(!target_info || target_info->HasTargets());
+  return target_info != nullptr;
 }
 
 bool AcceleratorManager::Process(const Accelerator& accelerator) {
-  auto map_iter = accelerators_.find(accelerator);
-  if (map_iter == accelerators_.end())
+  const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator);
+  if (!target_info)
     return false;
 
   // If the accelerator is in the map, the target list should not be empty.
-  AcceleratorTargetInfo& target_info = map_iter->second;
-  DCHECK(target_info.HasTargets());
+  DCHECK(target_info->HasTargets());
 
   // We have to copy the target list here, because processing the accelerator
   // event handler may modify the list.
-  AcceleratorTargetInfo target_info_copy(target_info);
+  AcceleratorTargetInfo target_info_copy(*target_info);
   return target_info_copy.TryProcess(accelerator);
 }
 
 bool AcceleratorManager::HasPriorityHandler(
     const Accelerator& accelerator) const {
-  auto map_iter = accelerators_.find(accelerator);
-  if (map_iter == accelerators_.end())
-    return false;
-
-  return map_iter->second.HasPriorityHandler();
+  const AcceleratorTargetInfo* target_info = accelerators_.Find(accelerator);
+  return target_info && target_info->HasPriorityHandler();
 }
 
 AcceleratorManager::AcceleratorTargetInfo::AcceleratorTargetInfo() = default;

ui/base/accelerators/accelerator_manager.h

@@ -11,6 +11,7 @@
 
 #include "base/component_export.h"
 #include "ui/base/accelerators/accelerator.h"
+#include "ui/base/accelerators/accelerator_map.h"
 
 namespace ui {
 
@@ -112,13 +113,16 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorManager {
     // Returns true if there are registered targets.
     bool HasTargets() const { return !targets_.empty(); }
 
+    // Returns the number of targets for this accelerator.
+    size_t size() const { return targets_.size(); }
+
    private:
     std::list<AcceleratorTarget*> targets_;
     bool has_priority_handler_ = false;
   };
 
   // The accelerators and associated targets.
-  std::map<Accelerator, AcceleratorTargetInfo> accelerators_;
+  AcceleratorMap<AcceleratorTargetInfo> accelerators_;
 };
 
 }  // namespace ui