From 29ef532a00372d2fc9d9950fcd7e27111907e9db Mon Sep 17 00:00:00 2001 From: Elly Fong-Jones Date: Thu, 30 Mar 2023 17:45:36 +0000 Subject: [PATCH] Revert "Settings Split: Implement keyboard policies in pref handler" This reverts commit 3db66e4a967c571e946d85e9a5f0f5e4a8fd595c. Reason for revert: introduced quite a lot of test failures After this change, quite a lot of tests are failing like this when run in parallel: Received signal 11 SEGV_MAPERR 000000000010 #0 0x5586cfbb1362 base::debug::CollectStackTrace() #1 0x5586cfb97043 base::debug::StackTrace::StackTrace() #2 0x5586cfbb0e5b base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f7b6f64a980 (/lib/x86_64-linux-gnu/libpthread-2.27.so+0x1297f) #4 0x5586d30583af ash::(anonymous namespace)::GetDefaultTopRowAreFKeysValue() #5 0x5586d3057d62 ash::(anonymous namespace)::GetDefaultKeyboardSettings() #6 0x5586d3057702 ash::KeyboardPrefHandlerImpl::InitializeKeyboardSettings() #7 0x5586d303eb8d ash::InputDeviceSettingsControllerImpl::OnKeyboardListUpdated() #8 0x5586d30481db base::internal::FunctorTraits<>::Invoke<>() #9 0x5586d30480b4 base::internal::Invoker<>::Run() #10 0x5586d3050506 _ZNKR4base17RepeatingCallbackIFvNSt2Cr6vectorIN2ui11InputDeviceENS1_9allocatorIS4_EEEENS2_IjNS5_IjEEEEEE3RunES7_S9_ #11 0x5586d3050360 ash::InputDeviceNotifier<>::RefreshDevices() #12 0x5586d30500e1 ash::InputDeviceNotifier<>::InputDeviceNotifier() #13 0x5586d303e3fe ash::InputDeviceSettingsControllerImpl::Init() #14 0x5586d303e080 ash::InputDeviceSettingsControllerImpl::InputDeviceSettingsControllerImpl() #15 0x5586d2f83b5d ash::Shell::Init() #16 0x5586d2f83167 ash::Shell::CreateInstance() #17 0x5586cf57ea8a ash::AshTestHelper::SetUp() #18 0x5586cf57e769 ash::AshTestHelper::SetUp() #19 0x5586ce66768e BrowserWithTestWindowTest::SetUp() #20 0x5586c8e0f37c testing::Test::Run() and then passing on retry. See https://chromium-swarm.appspot.com/task?id=6148891f87a85710&w=true Also reverting for the dependency for reverting https://chromium-review.googlesource.com/c/chromium/src/+/4375455 for crbug.com/1429313. Original change's description: > 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 > Commit-Queue: David Padlipsky > Cr-Commit-Position: refs/heads/main@{#1123980} Bug: b/241965700, 1429313 Change-Id: Id48919b445d28ce808d7f7c18c822338e31e2c42 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4384674 Commit-Queue: Hiroshige Hayashizaki Owners-Override: Hiroshige Hayashizaki Reviewed-by: Collin Baker Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/heads/main@{#1124303} --- .../input_device_settings_controller_impl.cc | 23 ++-- ...put_device_settings_controller_unittest.cc | 7 +- .../input_device_settings_utils.cc | 63 +---------- .../input_device_settings_utils.h | 59 ---------- .../pref_handlers/keyboard_pref_handler.h | 17 +-- .../keyboard_pref_handler_impl.cc | 103 ++++++++--------- .../keyboard_pref_handler_impl.h | 8 +- .../keyboard_pref_handler_unittest.cc | 106 +----------------- 8 files changed, 72 insertions(+), 314 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 index 826ab3243b461a..0ea4287fc06fde 100644 --- 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,9 +228,8 @@ void InputDeviceSettingsControllerImpl::OnActiveUserPrefServiceChanged( void InputDeviceSettingsControllerImpl::RefreshAllDeviceSettings() { settings_refresh_pending_ = false; for (const auto& [id, keyboard] : keyboards_) { - keyboard_pref_handler_->InitializeKeyboardSettings( - active_pref_service_, policy_handler_->keyboard_policies(), - keyboard.get()); + keyboard_pref_handler_->InitializeKeyboardSettings(active_pref_service_, + keyboard.get()); metrics_manager_->RecordKeyboardInitialMetrics(*keyboard); DispatchKeyboardSettingsChanged(id); } @@ -252,12 +251,8 @@ void InputDeviceSettingsControllerImpl::RefreshAllDeviceSettings() { } void InputDeviceSettingsControllerImpl::OnKeyboardPoliciesChanged() { - for (const auto& [id, keyboard] : keyboards_) { - keyboard_pref_handler_->InitializeKeyboardSettings( - active_pref_service_, policy_handler_->keyboard_policies(), - keyboard.get()); - DispatchKeyboardSettingsChanged(id); - } + // TODO(dpad): Implement re-initialization of keyboard settings when the + // policies change. } const mojom::KeyboardSettings* @@ -360,9 +355,8 @@ void InputDeviceSettingsControllerImpl::SetKeyboardSettings( return; } found_keyboard.settings = settings.Clone(); - keyboard_pref_handler_->UpdateKeyboardSettings( - active_pref_service_, policy_handler_->keyboard_policies(), - found_keyboard); + keyboard_pref_handler_->UpdateKeyboardSettings(active_pref_service_, + 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. @@ -589,9 +583,8 @@ 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_, policy_handler_->keyboard_policies(), - mojom_keyboard.get()); + keyboard_pref_handler_->InitializeKeyboardSettings(active_pref_service_, + 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 index d726cf7d691155..38ee3ef5ab273d 100644 --- 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,15 +52,12 @@ constexpr char kUserEmail2[] = "joy@abc.com"; class FakeKeyboardPrefHandler : public KeyboardPrefHandler { public: - void InitializeKeyboardSettings( - PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, - mojom::Keyboard* keyboard) override { + void InitializeKeyboardSettings(PrefService* pref_service, + 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 index 44a3dd95d9560b..a3c45fedb33033 100644 --- a/ash/system/input_device_settings/input_device_settings_utils.cc +++ b/ash/system/input_device_settings/input_device_settings_utils.cc @@ -4,17 +4,14 @@ #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 { +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. @@ -23,16 +20,6 @@ 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. @@ -47,52 +34,4 @@ std::string BuildDeviceKey(const ui::InputDevice& device) { {HexEncode(device.vendor_id), ":", HexEncode(device.product_id)}); } -template -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 index 6318609ed82f14..430a4e25ac574e 100644 --- a/ash/system/input_device_settings/input_device_settings_utils.h +++ b/ash/system/input_device_settings/input_device_settings_utils.h @@ -6,9 +6,6 @@ #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 { @@ -19,62 +16,6 @@ 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 -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 index bf4a73d1582f50..0fc5ef231bab0f 100644 --- 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,20 +18,15 @@ class ASH_EXPORT KeyboardPrefHandler { virtual ~KeyboardPrefHandler() = default; // Initializes device settings in prefs and update the `settings` member of - // 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; + // 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; // Updates device settings stored in prefs to match the values in // `keyboard.settings`. - virtual void UpdateKeyboardSettings( - PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, - const mojom::Keyboard& keyboard) = 0; + virtual void UpdateKeyboardSettings(PrefService* pref_service, + const mojom::Keyboard& keyboard) = 0; }; } // namespace ash diff --git a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.cc b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.cc index 457aa8e3bb13d6..c9107f037830c5 100644 --- a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.cc +++ b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.cc @@ -58,28 +58,15 @@ static constexpr auto kMetaKeyMapping = {mojom::MetaKey::kCommand, ::prefs::kLanguageRemapExternalCommandKeyTo}}); -bool GetDefaultTopRowAreFKeysValue( - const mojom::KeyboardPolicies& keyboard_policies, - const mojom::Keyboard& keyboard) { - if (keyboard_policies.top_row_are_fkeys_policy && - keyboard_policies.top_row_are_fkeys_policy->policy_status == - mojom::PolicyStatus::kRecommended) { - return keyboard_policies.top_row_are_fkeys_policy->value; - } - - return keyboard.is_external ? kDefaultTopRowAreFKeysExternal - : kDefaultTopRowAreFKeys; -} - -mojom::KeyboardSettingsPtr GetDefaultKeyboardSettings( - const mojom::KeyboardPolicies& keyboard_policies, - const mojom::Keyboard& keyboard) { +mojom::KeyboardSettingsPtr GetDefaultKeyboardSettings(bool is_external, + mojom::MetaKey meta_key) { mojom::KeyboardSettingsPtr settings = mojom::KeyboardSettings::New(); settings->suppress_meta_fkey_rewrites = kDefaultSuppressMetaFKeyRewrites; + // This setting should be enabled by default for external keyboards. settings->top_row_are_fkeys = - GetDefaultTopRowAreFKeysValue(keyboard_policies, keyboard); + is_external ? kDefaultTopRowAreFKeysExternal : kDefaultTopRowAreFKeys; // Switch control and command for Apple keyboards. - if (keyboard.meta_key == mojom::MetaKey::kCommand) { + if (meta_key == mojom::MetaKey::kCommand) { settings->modifier_remappings[ui::mojom::ModifierKey::kControl] = ui::mojom::ModifierKey::kMeta; settings->modifier_remappings[ui::mojom::ModifierKey::kMeta] = @@ -116,17 +103,15 @@ GetModifierRemappings(PrefService* prefs, const mojom::Keyboard& keyboard) { mojom::KeyboardSettingsPtr GetKeyboardSettingsFromGlobalPrefs( PrefService* prefs, - const mojom::KeyboardPolicies& keyboard_policies, const mojom::Keyboard& keyboard, ForceKeyboardSettingPersistence& force_persistence) { mojom::KeyboardSettingsPtr settings = mojom::KeyboardSettings::New(); const auto* top_row_are_fkeys_preference = prefs->GetUserPrefValue(prefs::kSendFunctionKeys); - settings->top_row_are_fkeys = - top_row_are_fkeys_preference - ? top_row_are_fkeys_preference->GetBool() - : GetDefaultTopRowAreFKeysValue(keyboard_policies, keyboard); + settings->top_row_are_fkeys = top_row_are_fkeys_preference + ? top_row_are_fkeys_preference->GetBool() + : kDefaultTopRowAreFKeys; force_persistence.top_row_are_fkeys = top_row_are_fkeys_preference != nullptr; settings->suppress_meta_fkey_rewrites = kDefaultSuppressMetaFKeyRewrites; @@ -137,9 +122,17 @@ mojom::KeyboardSettingsPtr GetKeyboardSettingsFromGlobalPrefs( return settings; } +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; +} + mojom::KeyboardSettingsPtr RetrieveKeyboardSettings( PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, const mojom::Keyboard& keyboard, const base::Value::Dict& settings_dict) { mojom::KeyboardSettingsPtr settings = mojom::KeyboardSettings::New(); @@ -148,7 +141,8 @@ mojom::KeyboardSettingsPtr RetrieveKeyboardSettings( .value_or(kDefaultSuppressMetaFKeyRewrites); settings->top_row_are_fkeys = settings_dict.FindBool(prefs::kKeyboardSettingTopRowAreFKeys) - .value_or(GetDefaultTopRowAreFKeysValue(keyboard_policies, keyboard)); + .value_or(keyboard.is_external ? kDefaultTopRowAreFKeysExternal + : kDefaultTopRowAreFKeys); const auto* modifier_remappings_dict = settings_dict.FindDict(prefs::kKeyboardSettingModifierRemappings); @@ -157,8 +151,8 @@ mojom::KeyboardSettingsPtr RetrieveKeyboardSettings( } for (const auto [from, to] : *modifier_remappings_dict) { - // `from` must be a string which can be converted to an int and `to` must be - // an int. + // `from` must be a string which can be converted to an int and `to` must + // be an int. int from_int, to_int; if (!to.is_int() || !base::StringToInt(from, &from_int)) { LOG(ERROR) << "Unable to parse modifier remappings from prefs. From: " @@ -186,7 +180,6 @@ mojom::KeyboardSettingsPtr RetrieveKeyboardSettings( void UpdateKeyboardSettingsImpl( PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, const mojom::Keyboard& keyboard, const ForceKeyboardSettingPersistence& force_persistence) { DCHECK(keyboard.settings); @@ -200,20 +193,29 @@ void UpdateKeyboardSettingsImpl( // Populate `settings_dict` with all settings in `settings`. base::Value::Dict settings_dict; - if (ShouldPersistSetting(prefs::kKeyboardSettingSuppressMetaFKeyRewrites, - settings.suppress_meta_fkey_rewrites, - kDefaultSuppressMetaFKeyRewrites, - force_persistence.suppress_meta_fkey_rewrites, - existing_settings_dict)) { + // Settings should only be persisted if one or more of the following is true: + // - Setting was previously persisted to storage + // - `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. + // - Setting is different than the default, which means the user manually + // changed the value. + + if (ExistingSettingsHasValue(prefs::kKeyboardSettingSuppressMetaFKeyRewrites, + existing_settings_dict) || + force_persistence.suppress_meta_fkey_rewrites || + settings.suppress_meta_fkey_rewrites != + kDefaultSuppressMetaFKeyRewrites) { settings_dict.Set(prefs::kKeyboardSettingSuppressMetaFKeyRewrites, settings.suppress_meta_fkey_rewrites); } - if (ShouldPersistSetting( - keyboard_policies.top_row_are_fkeys_policy, - prefs::kKeyboardSettingTopRowAreFKeys, settings.top_row_are_fkeys, - GetDefaultTopRowAreFKeysValue(keyboard_policies, keyboard), - force_persistence.top_row_are_fkeys, existing_settings_dict)) { + if (ExistingSettingsHasValue(prefs::kKeyboardSettingTopRowAreFKeys, + existing_settings_dict) || + force_persistence.top_row_are_fkeys || + settings.top_row_are_fkeys != keyboard.is_external + ? kDefaultTopRowAreFKeysExternal + : kDefaultTopRowAreFKeys) { settings_dict.Set(prefs::kKeyboardSettingTopRowAreFKeys, settings.top_row_are_fkeys); } @@ -249,11 +251,10 @@ KeyboardPrefHandlerImpl::~KeyboardPrefHandlerImpl() = default; void KeyboardPrefHandlerImpl::InitializeKeyboardSettings( PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, mojom::Keyboard* keyboard) { if (!pref_service) { keyboard->settings = - GetDefaultKeyboardSettings(keyboard_policies, *keyboard); + GetDefaultKeyboardSettings(keyboard->is_external, keyboard->meta_key); return; } @@ -263,36 +264,26 @@ void KeyboardPrefHandlerImpl::InitializeKeyboardSettings( ForceKeyboardSettingPersistence force_persistence; if (settings_dict) { - keyboard->settings = RetrieveKeyboardSettings( - pref_service, keyboard_policies, *keyboard, *settings_dict); + keyboard->settings = + RetrieveKeyboardSettings(pref_service, *keyboard, *settings_dict); } else if (Shell::Get()->input_device_tracker()->WasDevicePreviouslyConnected( InputDeviceTracker::InputDeviceCategory::kKeyboard, keyboard->device_key)) { keyboard->settings = GetKeyboardSettingsFromGlobalPrefs( - pref_service, keyboard_policies, *keyboard, force_persistence); + pref_service, *keyboard, force_persistence); } else { keyboard->settings = - GetDefaultKeyboardSettings(keyboard_policies, *keyboard); + GetDefaultKeyboardSettings(keyboard->is_external, keyboard->meta_key); } DCHECK(keyboard->settings); - UpdateKeyboardSettingsImpl(pref_service, keyboard_policies, *keyboard, - force_persistence); - - if (keyboard_policies.top_row_are_fkeys_policy && - keyboard_policies.top_row_are_fkeys_policy->policy_status == - mojom::PolicyStatus::kManaged) { - keyboard->settings->top_row_are_fkeys = - keyboard_policies.top_row_are_fkeys_policy->value; - } + UpdateKeyboardSettingsImpl(pref_service, *keyboard, force_persistence); } void KeyboardPrefHandlerImpl::UpdateKeyboardSettings( PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, const mojom::Keyboard& keyboard) { - UpdateKeyboardSettingsImpl(pref_service, keyboard_policies, keyboard, - /*force_persistence=*/{}); + UpdateKeyboardSettingsImpl(pref_service, keyboard, /*force_persistence=*/{}); } } // namespace ash diff --git a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.h b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.h index c7a84b3eb85e3d..fe1875a899790f 100644 --- a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.h +++ b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_impl.h @@ -6,7 +6,6 @@ #define ASH_SYSTEM_INPUT_DEVICE_SETTINGS_PREF_HANDLERS_KEYBOARD_PREF_HANDLER_IMPL_H_ #include "ash/ash_export.h" -#include "ash/public/mojom/input_device_settings.mojom-forward.h" #include "ash/system/input_device_settings/pref_handlers/keyboard_pref_handler.h" #include "base/values.h" @@ -22,12 +21,9 @@ class ASH_EXPORT KeyboardPrefHandlerImpl : public KeyboardPrefHandler { ~KeyboardPrefHandlerImpl() override; // KeyboardPrefHandler: - void InitializeKeyboardSettings( - PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, - mojom::Keyboard* keyboard) override; + void InitializeKeyboardSettings(PrefService* pref_service, + mojom::Keyboard* keyboard) override; void UpdateKeyboardSettings(PrefService* pref_service, - const mojom::KeyboardPolicies& keyboard_policies, const mojom::Keyboard& keyboard) override; }; diff --git a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_unittest.cc b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_unittest.cc index c4680785ce0df9..42d4f2f49f7786 100644 --- a/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_unittest.cc +++ b/ash/system/input_device_settings/pref_handlers/keyboard_pref_handler_unittest.cc @@ -165,8 +165,7 @@ class KeyboardPrefHandlerTest : public AshTestBase { keyboard->settings = settings.Clone(); keyboard->device_key = device_key; - pref_handler_->UpdateKeyboardSettings(pref_service_.get(), - /*keyboard_policies=*/{}, *keyboard); + pref_handler_->UpdateKeyboardSettings(pref_service_.get(), *keyboard); } mojom::KeyboardSettingsPtr CallInitializeKeyboardSettings( @@ -174,16 +173,16 @@ class KeyboardPrefHandlerTest : public AshTestBase { mojom::KeyboardPtr keyboard = mojom::Keyboard::New(); keyboard->device_key = device_key; - pref_handler_->InitializeKeyboardSettings( - pref_service_.get(), /*keyboard_policies=*/{}, keyboard.get()); + pref_handler_->InitializeKeyboardSettings(pref_service_.get(), + keyboard.get()); return std::move(keyboard->settings); } mojom::KeyboardSettingsPtr CallInitializeKeyboardSettings( const mojom::Keyboard& keyboard) { const auto keyboard_ptr = keyboard.Clone(); - pref_handler_->InitializeKeyboardSettings( - pref_service_.get(), /*keyboard_policies=*/{}, keyboard_ptr.get()); + pref_handler_->InitializeKeyboardSettings(pref_service_.get(), + keyboard_ptr.get()); return std::move(keyboard_ptr->settings); } @@ -322,8 +321,7 @@ TEST_F(KeyboardPrefHandlerTest, NewSettingAddedRoundTrip) { TEST_F(KeyboardPrefHandlerTest, DefaultSettingsWhenPrefServiceNull) { mojom::Keyboard keyboard; keyboard.device_key = kKeyboardKey1; - pref_handler_->InitializeKeyboardSettings(nullptr, /*keyboard_policies=*/{}, - &keyboard); + pref_handler_->InitializeKeyboardSettings(nullptr, &keyboard); EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); } @@ -516,98 +514,6 @@ TEST_F(KeyboardPrefHandlerTest, DefaultNotPersistedUntilUpdated) { *settings_dict); } -TEST_F(KeyboardPrefHandlerTest, - NewKeyboard_ManagedEnterprisePolicy_GetsDefaults) { - mojom::KeyboardPolicies policies; - policies.top_row_are_fkeys_policy = mojom::InputDeviceSettingsPolicy::New( - mojom::PolicyStatus::kManaged, !kDefaultTopRowAreFKeys); - - mojom::Keyboard keyboard; - keyboard.device_key = kKeyboardKey1; - - pref_handler_->InitializeKeyboardSettings(pref_service_.get(), policies, - &keyboard); - - EXPECT_EQ(!kDefaultTopRowAreFKeys, keyboard.settings->top_row_are_fkeys); - keyboard.settings->top_row_are_fkeys = kDefaultTopRowAreFKeys; - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - const auto* settings_dict = GetSettingsDictForDeviceKey(kKeyboardKey1); - EXPECT_FALSE(settings_dict->contains(prefs::kKeyboardSettingTopRowAreFKeys)); -} - -TEST_F(KeyboardPrefHandlerTest, - NewKeyboard_RecommendedEnterprisePolicy_GetsDefaults) { - mojom::KeyboardPolicies policies; - policies.top_row_are_fkeys_policy = mojom::InputDeviceSettingsPolicy::New( - mojom::PolicyStatus::kRecommended, !kDefaultTopRowAreFKeys); - - mojom::Keyboard keyboard; - keyboard.device_key = kKeyboardKey1; - - pref_handler_->InitializeKeyboardSettings(pref_service_.get(), policies, - &keyboard); - - EXPECT_EQ(!kDefaultTopRowAreFKeys, keyboard.settings->top_row_are_fkeys); - keyboard.settings->top_row_are_fkeys = kDefaultTopRowAreFKeys; - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - const auto* settings_dict = GetSettingsDictForDeviceKey(kKeyboardKey1); - EXPECT_FALSE(settings_dict->contains(prefs::kKeyboardSettingTopRowAreFKeys)); -} - -TEST_F(KeyboardPrefHandlerTest, - ExistingKeyboard_RecommendedEnterprisePolicy_GetsNewPolicy) { - mojom::KeyboardPolicies policies; - policies.top_row_are_fkeys_policy = mojom::InputDeviceSettingsPolicy::New( - mojom::PolicyStatus::kRecommended, !kDefaultTopRowAreFKeys); - - mojom::Keyboard keyboard; - keyboard.device_key = kKeyboardKey1; - - pref_handler_->InitializeKeyboardSettings( - pref_service_.get(), /*keyboard_policies=*/{}, &keyboard); - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - pref_handler_->InitializeKeyboardSettings(pref_service_.get(), policies, - &keyboard); - EXPECT_EQ(!kDefaultTopRowAreFKeys, keyboard.settings->top_row_are_fkeys); - keyboard.settings->top_row_are_fkeys = kDefaultTopRowAreFKeys; - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - const auto* settings_dict = GetSettingsDictForDeviceKey(kKeyboardKey1); - EXPECT_FALSE(settings_dict->contains(prefs::kKeyboardSettingTopRowAreFKeys)); -} - -TEST_F(KeyboardPrefHandlerTest, - ExistingKeyboard_ManagedEnterprisePolicy_GetsNewPolicy) { - mojom::KeyboardPolicies policies; - policies.top_row_are_fkeys_policy = mojom::InputDeviceSettingsPolicy::New( - mojom::PolicyStatus::kManaged, !kDefaultTopRowAreFKeys); - - mojom::Keyboard keyboard; - keyboard.device_key = kKeyboardKey1; - - pref_handler_->InitializeKeyboardSettings( - pref_service_.get(), /*keyboard_policies=*/{}, &keyboard); - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - keyboard.settings->top_row_are_fkeys = !kDefaultTopRowAreFKeys; - CallUpdateKeyboardSettings(kKeyboardKey1, *keyboard.settings); - - pref_handler_->InitializeKeyboardSettings(pref_service_.get(), policies, - &keyboard); - EXPECT_EQ(!kDefaultTopRowAreFKeys, keyboard.settings->top_row_are_fkeys); - keyboard.settings->top_row_are_fkeys = kDefaultTopRowAreFKeys; - EXPECT_EQ(kKeyboardSettingsDefault, *keyboard.settings); - - const auto* settings_dict = GetSettingsDictForDeviceKey(kKeyboardKey1); - EXPECT_TRUE(settings_dict->contains(prefs::kKeyboardSettingTopRowAreFKeys)); - EXPECT_EQ( - !kDefaultTopRowAreFKeys, - settings_dict->FindBool(prefs::kKeyboardSettingTopRowAreFKeys).value()); -} - class KeyboardSettingsPrefConversionTest : public KeyboardPrefHandlerTest, public testing::WithParamInterface<