Settings Split: Move ModifierKeys enum to ui/chromeos/events/mojom

In EventRewriterChromeOS, `kModifierRemappingNeoMod3` was reworked
slightly to more gracefully handle finding the remapping for de:neo's
mod3.

Bug: b/241965700
Change-Id: Ibf6cb492df1c1c2c0cbb90ac9c0ee6113e8ca5ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210122
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: David Padlipsky <dpad@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103948}

ash/events/accessibility_event_rewriter_unittest.cc

@@ -12,12 +12,13 @@
 #include "ash/public/cpp/accessibility_event_rewriter_delegate.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "base/check_op.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/base/ime/ash/fake_ime_keyboard.h"
 #include "ui/chromeos/events/event_rewriter_chromeos.h"
-#include "ui/chromeos/events/modifier_key.h"
+#include "ui/chromeos/events/mojom/modifier_key.mojom-shared.h"
 #include "ui/chromeos/events/pref_names.h"
 #include "ui/events/devices/device_data_manager_test_api.h"
 #include "ui/events/event.h"
@@ -134,7 +135,8 @@ class ChromeVoxAccessibilityEventRewriterTest
   }
 
   void SetModifierRemapping(const std::string& pref_name,
-                            ui::chromeos::ModifierKey value) {
+                            ui::mojom::ModifierKey value) {
+    DCHECK_NE(ui::mojom::ModifierKey::kIsoLevel5ShiftMod3, value);
     modifier_remapping_[pref_name] = static_cast<int>(value);
   }
 
@@ -360,7 +362,7 @@ TEST_F(ChromeVoxAccessibilityEventRewriterTest,
 
   // Map Control key to Search.
   SetModifierRemapping(prefs::kLanguageRemapControlKeyTo,
-                       ui::chromeos::ModifierKey::kSearchKey);
+                       ui::mojom::ModifierKey::kMeta);
 
   // Anything with Search gets captured.
   generator_->PressKey(ui::VKEY_CONTROL, ui::EF_CONTROL_DOWN);
@@ -541,7 +543,8 @@ class SwitchAccessAccessibilityEventRewriterTest
   }
 
   void SetModifierRemapping(const std::string& pref_name,
-                            ui::chromeos::ModifierKey value) {
+                            ui::mojom::ModifierKey value) {
+    DCHECK_NE(ui::mojom::ModifierKey::kIsoLevel5ShiftMod3, value);
     modifier_remapping_[pref_name] = static_cast<int>(value);
   }
 
@@ -794,7 +797,7 @@ TEST_F(SwitchAccessAccessibilityEventRewriterTest, RespectsModifierRemappings) {
 
   // Map Control key to Alt.
   SetModifierRemapping(prefs::kLanguageRemapControlKeyTo,
-                       ui::chromeos::ModifierKey::kAltKey);
+                       ui::mojom::ModifierKey::kAlt);
 
   // Send a key event for Control.
   generator_->PressKey(ui::VKEY_CONTROL, ui::EF_CONTROL_DOWN,

ash/public/mojom/BUILD.gn

@@ -25,6 +25,7 @@ mojom("mojom") {
     "//mojo/public/mojom/base",
     "//services/preferences/public/mojom",
     "//skia/public/mojom",
+    "//ui/chromeos/events/mojom",
     "//ui/gfx/geometry/mojom",
     "//ui/gfx/image/mojom",
     "//ui/gfx/range/mojom",

ash/public/mojom/input_device_settings.mojom

@@ -5,6 +5,7 @@
 module ash.mojom;
 
 import "mojo/public/mojom/base/time.mojom";
+import "ui/chromeos/events/mojom/modifier_key.mojom";
 
 // MetaKey denotes all the possible options deducable from the users external or
 // internal keyboard. Used to show the correct key to the user in the settings
@@ -16,22 +17,6 @@ enum MetaKey {
   kCommand
 };
 
-// ModifierKey contains the set of physical keys supported for modifier
-// remapping.
-enum ModifierKey {
-  kMeta = 0,
-  kControl = 1,
-  kAlt = 2,
-  // kVoid is used as a key that has no action. This key does not physically
-  // exist on a keyboard, but a user may remap any modifier to function like a
-  // void key.
-  kVoid = 3,
-  kCapsLock = 4,
-  kEscape = 5,
-  kBackspace = 6,
-  kAssistant = 7,
-};
-
 // Contains all information needed to display, apply, and update keyboard
 // settings.
 struct Keyboard {
@@ -50,13 +35,13 @@ struct Keyboard {
   // Meta key type (launcher, search, etc) for this keyboard.
   MetaKey meta_key;
   // List of modifier keys (caps lock, assistant, etc) present on this device.
-  array<ModifierKey> modifier_keys;
+  array<ui.mojom.ModifierKey> modifier_keys;
   KeyboardSettings settings;
 };
 
 // Contains all existing keyboard settings available for use.
 struct KeyboardSettings {
-  map<ModifierKey, ModifierKey> modifier_remappings;
+  map<ui.mojom.ModifierKey, ui.mojom.ModifierKey> modifier_remappings;
   bool top_row_are_fkeys;
   bool suppress_meta_fkey_rewrites;
   bool auto_repeat_enabled;

ash/system/caps_lock_notification_controller.cc

@@ -19,7 +19,7 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "ui/base/l10n/l10n_util.h"
-#include "ui/chromeos/events/modifier_key.h"
+#include "ui/chromeos/events/mojom/modifier_key.mojom.h"
 #include "ui/chromeos/events/pref_names.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
@@ -93,7 +93,7 @@ bool CapsLockNotificationController::IsSearchKeyMappedToCapsLock() {
   // to worry about sync changing the pref while the menu or notification is
   // visible.
   return prefs->GetInteger(prefs::kLanguageRemapSearchKeyTo) ==
-         static_cast<int>(ui::chromeos::ModifierKey::kCapsLockKey);
+         static_cast<int>(ui::mojom::ModifierKey::kCapsLock);
 }
 
 void CapsLockNotificationController::OnCapsLockChanged(bool enabled) {

ash/system/input_device_settings/input_device_settings_utils.cc

@@ -4,10 +4,10 @@
 
 #include "ash/system/input_device_settings/input_device_settings_utils.h"
 
-#include "ash/public/mojom/input_device_settings.mojom.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "ui/chromeos/events/mojom/modifier_key.mojom.h"
 
 namespace ash {
 
@@ -22,9 +22,11 @@ std::string HexEncode(uint16_t v) {
 }
 }  // namespace
 
+// `kIsoLevel5ShiftMod3` is not a valid modifier value.
 bool IsValidModifier(int val) {
-  return val >= static_cast<int>(mojom::ModifierKey::kMinValue) &&
-         val <= static_cast<int>(mojom::ModifierKey::kMaxValue);
+  return val >= static_cast<int>(ui::mojom::ModifierKey::kMinValue) &&
+         val <= static_cast<int>(ui::mojom::ModifierKey::kMaxValue) &&
+         val != static_cast<int>(ui::mojom::ModifierKey::kIsoLevel5ShiftMod3);
 }
 
 std::string BuildDeviceKey(const ui::InputDevice& device) {

ash/system/input_device_settings/input_device_settings_utils.h

@@ -10,7 +10,7 @@
 
 namespace ash {
 
-// Checks if a given value is within the bounds set by `mojom::ModifierKey`.
+// Checks if a given value is within the bounds set by `ui::mojom::ModifierKey`.
 bool IsValidModifier(int val);
 
 // Builds `device_key` for use in storing device settings in prefs.

ash/system/input_device_settings/keyboard_modifier_metrics_recorder.cc

@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "ash/constants/ash_features.h"
-#include "ash/public/mojom/input_device_settings.mojom-shared.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/system/input_device_settings/input_device_settings_utils.h"
@@ -21,6 +20,7 @@
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "ui/chromeos/events/mojom/modifier_key.mojom.h"
 
 namespace ash {
 namespace {
@@ -66,10 +66,12 @@ constexpr int kNumModifiers =
 // 32-bit int will fit without any overflow or UB.
 // Modifier hash is limited to 32 bits as metrics can only handle 32 bit ints.
 static_assert((sizeof(int32_t) * 8) >= (kModifierHashWidth * kNumModifiers));
-static_assert(static_cast<int>(mojom::ModifierKey::kMaxValue) <=
+// `kIsoLevel5ShiftMod3` is not a valid modifier for the purposes of these
+// metrics so there is 1 less modifier than the max value.
+static_assert(static_cast<int>(ui::mojom::ModifierKey::kMaxValue) - 1 <=
               kMaxModifierValue);
 
-constexpr mojom::ModifierKey GetDefaultModifier(size_t index) {
+constexpr ui::mojom::ModifierKey GetDefaultModifier(size_t index) {
   return KeyboardModifierMetricsRecorder::kKeyboardModifierPrefs[index]
       .default_modifier_key;
 }
@@ -144,7 +146,7 @@ void KeyboardModifierMetricsRecorder::OnActiveUserPrefServiceChanged(
       const int value = pref_member->GetValue();
       DCHECK(IsValidModifier(value));
       RecordModifierRemappingInit(index,
-                                  static_cast<mojom::ModifierKey>(value));
+                                  static_cast<ui::mojom::ModifierKey>(value));
     }
     RecordModifierRemappingHash();
   }
@@ -160,12 +162,13 @@ void KeyboardModifierMetricsRecorder::OnModifierRemappingChanged(
 
   int value = pref_member->GetValue();
   DCHECK(IsValidModifier(value));
-  RecordModifierRemappingChanged(index, static_cast<mojom::ModifierKey>(value));
+  RecordModifierRemappingChanged(index,
+                                 static_cast<ui::mojom::ModifierKey>(value));
 }
 
 void KeyboardModifierMetricsRecorder::RecordModifierRemappingChanged(
     size_t index,
-    mojom::ModifierKey modifier_key) {
+    ui::mojom::ModifierKey modifier_key) {
   const std::string changed_metric_name = base::StrCat(
       {kModifierMetricPrefix, kKeyboardModifierPrefs[index].key_name,
        kModifierMetricIndividualChangedSuffix});
@@ -174,8 +177,9 @@ void KeyboardModifierMetricsRecorder::RecordModifierRemappingChanged(
 
 void KeyboardModifierMetricsRecorder::RecordModifierRemappingInit(
     size_t index,
-    mojom::ModifierKey modifier_key) {
+    ui::mojom::ModifierKey modifier_key) {
   DCHECK_LT(index, std::size(kKeyboardModifierPrefs));
+
   // Skip publishing the metric if the pref is set to its default value.
   if (GetDefaultModifier(index) == modifier_key) {
     return;
@@ -202,7 +206,7 @@ void KeyboardModifierMetricsRecorder::RecordModifierRemappingHash() {
     const int value = pref_members_[i]->GetValue();
 
     // Check that shifting and adding value will not overflow `hash`.
-    DCHECK(value <= kMaxModifierValue && value >= 0);
+    DCHECK(IsValidModifier(value));
     DCHECK(hash < (1u << ((sizeof(uint32_t) * 8u) - kModifierHashWidth)));
 
     hash <<= kModifierHashWidth;

ash/system/input_device_settings/keyboard_modifier_metrics_recorder.h

@@ -10,9 +10,10 @@
 
 #include "ash/ash_export.h"
 #include "ash/public/cpp/session/session_observer.h"
-#include "ash/public/mojom/input_device_settings.mojom-shared.h"
+#include "base/containers/flat_set.h"
 #include "components/account_id/account_id.h"
 #include "components/prefs/pref_member.h"
+#include "ui/chromeos/events/mojom/modifier_key.mojom.h"
 #include "ui/chromeos/events/pref_names.h"
 
 class PrefService;
@@ -29,24 +30,25 @@ class ASH_EXPORT KeyboardModifierMetricsRecorder : public SessionObserver {
   static constexpr struct {
     const char* key_name;
     const char* pref_name;
-    mojom::ModifierKey default_modifier_key;
+    ui::mojom::ModifierKey default_modifier_key;
   } kKeyboardModifierPrefs[] = {
-      {"Alt", ::prefs::kLanguageRemapAltKeyTo, mojom::ModifierKey::kAlt},
+      {"Alt", ::prefs::kLanguageRemapAltKeyTo, ui::mojom::ModifierKey::kAlt},
       {"Control", ::prefs::kLanguageRemapControlKeyTo,
-       mojom::ModifierKey::kControl},
+       ui::mojom::ModifierKey::kControl},
       {"Escape", ::prefs::kLanguageRemapEscapeKeyTo,
-       mojom::ModifierKey::kEscape},
+       ui::mojom::ModifierKey::kEscape},
       {"Backspace", ::prefs::kLanguageRemapBackspaceKeyTo,
-       mojom::ModifierKey::kBackspace},
+       ui::mojom::ModifierKey::kBackspace},
       {"Assistant", ::prefs::kLanguageRemapAssistantKeyTo,
-       mojom::ModifierKey::kAssistant},
+       ui::mojom::ModifierKey::kAssistant},
       {"CapsLock", ::prefs::kLanguageRemapCapsLockKeyTo,
-       mojom::ModifierKey::kCapsLock},
+       ui::mojom::ModifierKey::kCapsLock},
       {"ExternalMeta", ::prefs::kLanguageRemapExternalMetaKeyTo,
-       mojom::ModifierKey::kMeta},
-      {"Search", ::prefs::kLanguageRemapSearchKeyTo, mojom::ModifierKey::kMeta},
+       ui::mojom::ModifierKey::kMeta},
+      {"Search", ::prefs::kLanguageRemapSearchKeyTo,
+       ui::mojom::ModifierKey::kMeta},
       {"ExternalCommand", ::prefs::kLanguageRemapExternalCommandKeyTo,
-       mojom::ModifierKey::kControl},
+       ui::mojom::ModifierKey::kControl},
   };
 
   KeyboardModifierMetricsRecorder();
@@ -68,9 +70,9 @@ class ASH_EXPORT KeyboardModifierMetricsRecorder : public SessionObserver {
   void ResetPrefMembers();
 
   void RecordModifierRemappingChanged(size_t index,
-                                      mojom::ModifierKey modifier_key);
+                                      ui::mojom::ModifierKey modifier_key);
   void RecordModifierRemappingInit(size_t index,
-                                   mojom::ModifierKey modifier_key);
+                                   ui::mojom::ModifierKey modifier_key);
 
   // TODO(dpad): Remove pref members once transitioned to per device settings.
   std::array<std::unique_ptr<IntegerPrefMember>,