Relax constraint on DomCode in accelerator_map

- Previously the map DCHECK'd when registering an accelerator
  with a DomCode
- Accelerators created from KeyEvents include the DomCode and that
  is required during the lookup path
- Change the map to set the DomCode to NONE when storing

Bug: 1174326
Test: ash_unittests --gtest_filter="Accelerator*"
Change-Id: I9a0b4dea57fa16b138c888cbaf6aa4c52c2e278a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2880646
Commit-Queue: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880626}

ui/base/accelerators/accelerator.h

@@ -93,6 +93,7 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator {
 
 #if defined(OS_CHROMEOS)
   DomCode code() const { return code_; }
+  void reset_code() { code_ = DomCode::NONE; }
 #endif
 
   // Sets the key state that triggers the accelerator. Default is PRESSED.

ui/base/accelerators/accelerator_map.h

@@ -79,8 +79,13 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap {
 
   V& GetOrInsertDefault(const Accelerator& accelerator) {
 #if defined(OS_CHROMEOS)
-    // Cannot explicitly register an accelerator with a DomCode.
-    DCHECK_EQ(DomCode::NONE, accelerator.code());
+    // Ensure the DomCode is NONE before registering. The DomCode is only
+    // used during lookup to select the correct VKEY.
+    if (accelerator.code() != DomCode::NONE) {
+      Accelerator accelerator_copy = accelerator;
+      accelerator_copy.reset_code();
+      return map_[accelerator_copy];
+    }
 #endif
     return map_[accelerator];
   }
@@ -94,8 +99,16 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap {
   // accelerator was already in the map.
   void InsertNew(const std::pair<const Accelerator, V>& value) {
 #if defined(OS_CHROMEOS)
-    // Cannot explicitly register an accelerator with a DomCode.
-    DCHECK_EQ(DomCode::NONE, value.first.code());
+    // Ensure the DomCode is NONE before registering. The DomCode is only
+    // used during lookup to select the correct VKEY.
+    if (value.first.code() != DomCode::NONE) {
+      Accelerator accelerator_copy = value.first;
+      accelerator_copy.reset_code();
+      auto value_copy = std::make_pair(accelerator_copy, value.second);
+      auto result = map_.insert(value_copy);
+      DCHECK(result.second);
+      return;
+    }
 #endif
     auto result = map_.insert(value);
     DCHECK(result.second);
@@ -154,12 +167,15 @@ class COMPONENT_EXPORT(UI_BASE) AcceleratorMap {
     return Accelerator(lookup_key_code, DomCode::NONE, accelerator.modifiers(),
                        accelerator.key_state());
   }
-#endif
+#endif  // defined(OS_CHROMEOS)
 
   const_iterator FindImpl(const Accelerator& accelerator) const {
 #if defined(OS_CHROMEOS)
-    return map_.find(RemapAcceleratorForLookup(accelerator));
-#endif
+    auto iter = map_.find(RemapAcceleratorForLookup(accelerator));
+    // Sanity check that a DomCode was never inserted into the map.
+    DCHECK(iter == map_.end() || iter->first.code() == DomCode::NONE);
+    return iter;
+#endif  // defined(OS_CHROMEOS)
 
     return map_.find(accelerator);
   }

ui/base/accelerators/accelerator_map_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/accelerators/accelerator.h"
+#include "ui/events/event.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 
 namespace ui {
@@ -206,6 +207,27 @@ TEST(AcceleratorMapTest, PositionalLookupVkeyMatchOnlyRegisteredDomCodeIsNone) {
   EXPECT_TRUE(IsValidMatch(&m, pressed, expected));
 }
 
+// When an accelerator is inserted to the map, if it contains a DomCode it
+// should be stripped out.
+TEST(AcceleratorMapTest, DomCodesStrippedWhenInserted) {
+  AcceleratorMap<int> m;
+  m.set_use_positional_lookup(true);
+
+  // Verify the accelerator has a DomCode and insert it.
+  Accelerator accelerator(ui::VKEY_F, DomCode::US_F,
+                          ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN);
+  EXPECT_EQ(accelerator.code(), DomCode::US_F);
+  const int expected = 77;
+  m.InsertNew(std::make_pair(accelerator, expected));
+
+  // Reset the DomCode on the accelerator and perform a lookup and verify
+  // that it can still be found.
+  accelerator.reset_code();
+  auto* value = m.Find(accelerator);
+  ASSERT_TRUE(value);
+  EXPECT_EQ(*value, expected);
+}
+
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace

ui/base/accelerators/accelerator_unittest.cc

@@ -7,8 +7,10 @@
 #include <string>
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/ui_base_features.h"
 #include "ui/events/event.h"
 
 namespace ui {
@@ -74,4 +76,35 @@ TEST(AcceleratorTest, ShortcutTextForUnknownKey) {
   EXPECT_EQ(std::u16string(), accelerator.GetShortcutText());
 }
 
+TEST(AcceleratorTest, ConversionFromKeyEvent) {
+  ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_F,
+                         ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN);
+  Accelerator accelerator(key_event);
+
+  EXPECT_EQ(accelerator.key_code(), ui::VKEY_F);
+  EXPECT_EQ(accelerator.modifiers(), ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN);
+}
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+TEST(AcceleratorTest, ConversionFromKeyEvent_Ash) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      ::features::kImprovedKeyboardShortcuts);
+
+  ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_F,
+                         ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN);
+  Accelerator accelerator(key_event);
+
+  EXPECT_EQ(accelerator.key_code(), ui::VKEY_F);
+  EXPECT_EQ(accelerator.modifiers(), ui::EF_ALT_DOWN | ui::EF_CONTROL_DOWN);
+
+  // Code is set when converting from a KeyEvent.
+  EXPECT_EQ(accelerator.code(), DomCode::US_F);
+
+  // Test resetting code.
+  accelerator.reset_code();
+  EXPECT_EQ(accelerator.code(), DomCode::NONE);
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
 }  // namespace ui