Skip to content

Commit

Permalink
Revert "Settings Split: Implement keyboard policies in pref handler"
Browse files Browse the repository at this point in the history 
This reverts commit 3db66e4.

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 <[email protected]>
> Commit-Queue: David Padlipsky <[email protected]>
> 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 <[email protected]>
Owners-Override: Hiroshige Hayashizaki <[email protected]>
Reviewed-by: Collin Baker <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1124303}
  • Loading branch information
Elly Fong-Jones authored and Chromium LUCI CQ committed Mar 30, 2023
1 parent 2dea335 commit 29ef532
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 314 deletions.
23 changes: 8 additions & 15 deletions ash/system/input_device_settings/input_device_settings_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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*
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 2 additions & 5 deletions ash/system/input_device_settings/input_device_settings_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,12 @@ constexpr char kUserEmail2[] = "[email protected]";

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_++;
}
Expand Down
63 changes: 1 addition & 62 deletions ash/system/input_device_settings/input_device_settings_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -47,52 +34,4 @@ 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
59 changes: 0 additions & 59 deletions ash/system/input_device_settings/input_device_settings_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 <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_
17 changes: 6 additions & 11 deletions ash/system/input_device_settings/pref_handlers/keyboard_pref_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 29ef532

Please sign in to comment.