Settings Split: Implement keyboard policies in pref handler

Bug: b/241965700 Change-Id: I5579395a53df6054e3a23bba3086e3b543aecc0c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4367666 Reviewed-by: Michael Checo <[email protected]> Commit-Queue: David Padlipsky <[email protected]> Cr-Commit-Position: refs/heads/main@{#1123980}
jamlee-t · Mar 30, 2023 · 3db66e4 · 3db66e4
1 parent 5269ebc
commit 3db66e4
Show file tree

Hide file tree

Showing 8 changed files with 314 additions and 72 deletions.
diff --git a/ash/system/input_device_settings/input_device_settings_controller_impl.cc b/ash/system/input_device_settings/input_device_settings_controller_impl.cc
@@ -228,8 +228,9 @@ void InputDeviceSettingsControllerImpl::OnActiveUserPrefServiceChanged(
 void InputDeviceSettingsControllerImpl::RefreshAllDeviceSettings() {
   settings_refresh_pending_ = false;
   for (const auto& [id, keyboard] : keyboards_) {
-    keyboard_pref_handler_->InitializeKeyboardSettings(active_pref_service_,
-                                                       keyboard.get());
+    keyboard_pref_handler_->InitializeKeyboardSettings(
+        active_pref_service_, policy_handler_->keyboard_policies(),
+        keyboard.get());
     metrics_manager_->RecordKeyboardInitialMetrics(*keyboard);
     DispatchKeyboardSettingsChanged(id);
   }
@@ -251,8 +252,12 @@ void InputDeviceSettingsControllerImpl::RefreshAllDeviceSettings() {
 }
 
 void InputDeviceSettingsControllerImpl::OnKeyboardPoliciesChanged() {
-  // TODO(dpad): Implement re-initialization of keyboard settings when the
-  // policies change.
+  for (const auto& [id, keyboard] : keyboards_) {
+    keyboard_pref_handler_->InitializeKeyboardSettings(
+        active_pref_service_, policy_handler_->keyboard_policies(),
+        keyboard.get());
+    DispatchKeyboardSettingsChanged(id);
+  }
 }
 
 const mojom::KeyboardSettings*
@@ -355,8 +360,9 @@ void InputDeviceSettingsControllerImpl::SetKeyboardSettings(
     return;
   }
   found_keyboard.settings = settings.Clone();
-  keyboard_pref_handler_->UpdateKeyboardSettings(active_pref_service_,
-                                                 found_keyboard);
+  keyboard_pref_handler_->UpdateKeyboardSettings(
+      active_pref_service_, policy_handler_->keyboard_policies(),
+      found_keyboard);
   DispatchKeyboardSettingsChanged(id);
   // Check the list of keyboards to see if any have the same |device_key|.
   // If so, their settings need to also be updated.
@@ -583,8 +589,9 @@ void InputDeviceSettingsControllerImpl::OnKeyboardListUpdated(
     // Get initial settings from the pref manager and generate our local storage
     // of the device.
     auto mojom_keyboard = BuildMojomKeyboard(keyboard);
-    keyboard_pref_handler_->InitializeKeyboardSettings(active_pref_service_,
-                                                       mojom_keyboard.get());
+    keyboard_pref_handler_->InitializeKeyboardSettings(
+        active_pref_service_, policy_handler_->keyboard_policies(),
+        mojom_keyboard.get());
     metrics_manager_->RecordKeyboardInitialMetrics(*mojom_keyboard);
     keyboards_.insert_or_assign(keyboard.id, std::move(mojom_keyboard));
     DispatchKeyboardConnected(keyboard.id);

diff --git a/ash/system/input_device_settings/input_device_settings_controller_unittest.cc b/ash/system/input_device_settings/input_device_settings_controller_unittest.cc
@@ -52,12 +52,15 @@ constexpr char kUserEmail2[] = "[email protected]";
 
 class FakeKeyboardPrefHandler : public KeyboardPrefHandler {
  public:
-  void InitializeKeyboardSettings(PrefService* pref_service,
-                                  mojom::Keyboard* keyboard) override {
+  void InitializeKeyboardSettings(
+      PrefService* pref_service,
+      const mojom::KeyboardPolicies& keyboard_policies,
+      mojom::Keyboard* keyboard) override {
     keyboard->settings = mojom::KeyboardSettings::New();
     num_keyboard_settings_initialized_++;
   }
   void UpdateKeyboardSettings(PrefService* pref_service,
+                              const mojom::KeyboardPolicies& keyboard_policies,
                               const mojom::Keyboard& keyboard) override {
     num_keyboard_settings_updated_++;
   }

diff --git a/ash/system/input_device_settings/input_device_settings_utils.cc b/ash/system/input_device_settings/input_device_settings_utils.cc
@@ -4,14 +4,17 @@
 
 #include "ash/system/input_device_settings/input_device_settings_utils.h"
 
+#include "ash/public/mojom/input_device_settings.mojom.h"
+#include "base/export_template.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/values.h"
 #include "ui/chromeos/events/mojom/modifier_key.mojom.h"
 
 namespace ash {
-
 namespace {
+
 std::string HexEncode(uint16_t v) {
   // Load the bytes into the bytes array in reverse order as hex number should
   // be read from left to right.
@@ -20,6 +23,16 @@ std::string HexEncode(uint16_t v) {
   bytes[0] = v >> 8;
   return base::ToLowerASCII(base::HexEncode(bytes));
 }
+
+bool ExistingSettingsHasValue(base::StringPiece setting_key,
+                              const base::Value::Dict* existing_settings_dict) {
+  if (!existing_settings_dict) {
+    return false;
+  }
+
+  return existing_settings_dict->Find(setting_key) != nullptr;
+}
+
 }  // namespace
 
 // `kIsoLevel5ShiftMod3` is not a valid modifier value.
@@ -34,4 +47,52 @@ std::string BuildDeviceKey(const ui::InputDevice& device) {
       {HexEncode(device.vendor_id), ":", HexEncode(device.product_id)});
 }
 
+template <typename T>
+bool ShouldPersistSetting(base::StringPiece setting_key,
+                          T new_value,
+                          T default_value,
+                          bool force_persistence,
+                          const base::Value::Dict* existing_settings_dict) {
+  return ExistingSettingsHasValue(setting_key, existing_settings_dict) ||
+         new_value != default_value || force_persistence;
+}
+
+bool ShouldPersistSetting(const mojom::InputDeviceSettingsPolicyPtr& policy,
+                          base::StringPiece setting_key,
+                          bool new_value,
+                          bool default_value,
+                          bool force_persistence,
+                          const base::Value::Dict* existing_settings_dict) {
+  if (force_persistence) {
+    return true;
+  }
+
+  if (!policy) {
+    return ShouldPersistSetting(setting_key, new_value, default_value,
+                                force_persistence, existing_settings_dict);
+  }
+
+  switch (policy->policy_status) {
+    case mojom::PolicyStatus::kRecommended:
+      return ExistingSettingsHasValue(setting_key, existing_settings_dict) ||
+             new_value != policy->value;
+    case mojom::PolicyStatus::kManaged:
+      return false;
+  }
+}
+
+template EXPORT_TEMPLATE_DEFINE(ASH_EXPORT) bool ShouldPersistSetting(
+    base::StringPiece setting_key,
+    bool new_value,
+    bool default_value,
+    bool force_persistence,
+    const base::Value::Dict* existing_settings_dict);
+
+template EXPORT_TEMPLATE_DEFINE(ASH_EXPORT) bool ShouldPersistSetting(
+    base::StringPiece setting_key,
+    int value,
+    int default_value,
+    bool force_persistence,
+    const base::Value::Dict* existing_settings_dict);
+
 }  // namespace ash
diff --git a/ash/system/input_device_settings/input_device_settings_utils.h b/ash/system/input_device_settings/input_device_settings_utils.h
@@ -6,6 +6,9 @@
 #define ASH_SYSTEM_INPUT_DEVICE_SETTINGS_INPUT_DEVICE_SETTINGS_UTILS_H_
 
 #include "ash/ash_export.h"
+#include "ash/public/mojom/input_device_settings.mojom-forward.h"
+#include "base/export_template.h"
+#include "base/values.h"
 #include "ui/events/devices/input_device.h"
 
 namespace ash {
@@ -16,6 +19,62 @@ bool IsValidModifier(int val);
 // Builds `device_key` for use in storing device settings in prefs.
 ASH_EXPORT std::string BuildDeviceKey(const ui::InputDevice& device);
 
+// Decides based on the existing settings storage and default value if the given
+// setting should be persisted.
+// Settings should be persisted if any of the following are true:
+// - Setting was previously persisted to storage
+// - Setting is different than the default, which means the user manually
+//   changed the value.
+// - `force_persistence` requires the setting to be persisted, this means this
+//   device is being transitioned from the old global settings to per-device
+//   settings and the user specified the specific value for this setting.
+template <typename T>
+bool ShouldPersistSetting(base::StringPiece setting_key,
+                          T new_value,
+                          T default_value,
+                          bool force_persistence,
+                          const ::base::Value::Dict* existing_settings_dict);
+
+// Decides based on the policy, existing settings storage, and default value if
+// the given setting should be persisted. Settings should be persisted if any of
+// the following are true when there is no policy:
+// - Setting was previously persisted to storage
+// - Setting is different than the default, which means the user manually
+//   changed the value.
+// - `force_persistence` requires the setting to be persisted, this means this
+//   device is being transitioned from the old global settings to per-device
+//   settings and the user specified the specific value for this setting.
+
+// If there is a managed policy, settings should only be persisted if
+// `force_peristence` is true.
+// If there is a recommended policy, settings should be persisted if any of the
+// following are true:
+// - Setting was previously persisted to storage
+// - Setting is going to be set to a different value than the policy recommended
+// value.
+ASH_EXPORT bool ShouldPersistSetting(
+    const mojom::InputDeviceSettingsPolicyPtr& policy,
+    base::StringPiece setting_key,
+    bool new_value,
+    bool default_value,
+    bool force_persistence,
+    const base::Value::Dict* existing_settings_dict);
+
+// Templates exported for each valid value type.
+extern template EXPORT_TEMPLATE_DECLARE(ASH_EXPORT) bool ShouldPersistSetting(
+    base::StringPiece setting_key,
+    bool new_value,
+    bool default_value,
+    bool force_persistence,
+    const base::Value::Dict* existing_settings_dict);
+
+extern template EXPORT_TEMPLATE_DECLARE(ASH_EXPORT) bool ShouldPersistSetting(
+    base::StringPiece setting_key,
+    int new_value,
+    int default_value,
+    bool force_persistence,
+    const base::Value::Dict* existing_settings_dict);
+
 }  // namespace ash
 
 #endif  // ASH_SYSTEM_INPUT_DEVICE_SETTINGS_INPUT_DEVICE_SETTINGS_UTILS_H_
diff --git a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler.h b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler.h
@@ -18,15 +18,20 @@ class ASH_EXPORT KeyboardPrefHandler {
   virtual ~KeyboardPrefHandler() = default;
 
   // Initializes device settings in prefs and update the `settings` member of
-  // the `mojom::Keyboard` object.
-  // If `pref_service` is null, sets the `settings` member to default settings.
-  virtual void InitializeKeyboardSettings(PrefService* pref_service,
-                                          mojom::Keyboard* keyboard) = 0;
+  // the `mojom::Keyboard` object. Respects all policies given by
+  // `keyboard_policies`. If `pref_service` is null, sets the `settings` member
+  // to default settings.
+  virtual void InitializeKeyboardSettings(
+      PrefService* pref_service,
+      const mojom::KeyboardPolicies& keyboard_policies,
+      mojom::Keyboard* keyboard) = 0;
 
   // Updates device settings stored in prefs to match the values in
   // `keyboard.settings`.
-  virtual void UpdateKeyboardSettings(PrefService* pref_service,
-                                      const mojom::Keyboard& keyboard) = 0;
+  virtual void UpdateKeyboardSettings(
+      PrefService* pref_service,
+      const mojom::KeyboardPolicies& keyboard_policies,
+      const mojom::Keyboard& keyboard) = 0;
 };
 
 }  // namespace ash