0

Introduce UserManagerBase::Delegate.

As explicit injection point, instead of inheritance.
As a first step. GetApplicationLocale().

BUG=278643115
TEST=Tryjob

Change-Id: Ice88e29acdb463f38de4e0985640e6de36f14ba5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5387484
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1277744}
This commit is contained in:
Hidehiko Abe
2024-03-25 17:27:12 +00:00
committed by Chromium LUCI CQ
parent ccaa402052
commit 3ef901b2c6
14 changed files with 126 additions and 24 deletions

@@ -2198,6 +2198,8 @@ source_set("ash") {
"login/users/default_user_image/default_user_images.h", "login/users/default_user_image/default_user_images.h",
"login/users/test_users.cc", "login/users/test_users.cc",
"login/users/test_users.h", "login/users/test_users.h",
"login/users/user_manager_delegate_impl.cc",
"login/users/user_manager_delegate_impl.h",
"login/version_info_updater.cc", "login/version_info_updater.cc",
"login/version_info_updater.h", "login/version_info_updater.h",
"login/version_updater/update_time_estimator.cc", "login/version_updater/update_time_estimator.cc",

@@ -46,6 +46,7 @@
#include "chrome/browser/ash/login/users/affiliation.h" #include "chrome/browser/ash/login/users/affiliation.h"
#include "chrome/browser/ash/login/users/chrome_user_manager_util.h" #include "chrome/browser/ash/login/users/chrome_user_manager_util.h"
#include "chrome/browser/ash/login/users/default_user_image/default_user_images.h" #include "chrome/browser/ash/login/users/default_user_image/default_user_images.h"
#include "chrome/browser/ash/login/users/user_manager_delegate_impl.h"
#include "chrome/browser/ash/policy/core/browser_policy_connector_ash.h" #include "chrome/browser/ash/policy/core/browser_policy_connector_ash.h"
#include "chrome/browser/ash/policy/core/device_local_account.h" #include "chrome/browser/ash/policy/core/device_local_account.h"
#include "chrome/browser/ash/policy/external_data/handlers/crostini_ansible_playbook_external_data_handler.h" #include "chrome/browser/ash/policy/external_data/handlers/crostini_ansible_playbook_external_data_handler.h"
@@ -290,6 +291,7 @@ ChromeUserManagerImpl::CreateChromeUserManager() {
ChromeUserManagerImpl::ChromeUserManagerImpl() ChromeUserManagerImpl::ChromeUserManagerImpl()
: UserManagerBase( : UserManagerBase(
std::make_unique<UserManagerDelegateImpl>(),
base::SingleThreadTaskRunner::HasCurrentDefault() base::SingleThreadTaskRunner::HasCurrentDefault()
? base::SingleThreadTaskRunner::GetCurrentDefault() ? base::SingleThreadTaskRunner::GetCurrentDefault()
: nullptr, : nullptr,
@@ -591,10 +593,6 @@ void ChromeUserManagerImpl::OnDeviceLocalAccountsChanged() {
// handled via the kAccountsPrefDeviceLocalAccounts device setting observer. // handled via the kAccountsPrefDeviceLocalAccounts device setting observer.
} }
const std::string& ChromeUserManagerImpl::GetApplicationLocale() const {
return g_browser_process->GetApplicationLocale();
}
bool ChromeUserManagerImpl::IsEnterpriseManaged() const { bool ChromeUserManagerImpl::IsEnterpriseManaged() const {
policy::BrowserPolicyConnectorAsh* connector = policy::BrowserPolicyConnectorAsh* connector =
g_browser_process->platform_part()->browser_policy_connector_ash(); g_browser_process->platform_part()->browser_policy_connector_ash();

@@ -117,7 +117,6 @@ class ChromeUserManagerImpl
void OnProfileWillBeDestroyed(Profile* profile) override; void OnProfileWillBeDestroyed(Profile* profile) override;
protected: protected:
const std::string& GetApplicationLocale() const override;
void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override; void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override;
void NotifyOnLogin() override; void NotifyOnLogin() override;
void NotifyUserAddedToSession(const user_manager::User* added_user, void NotifyUserAddedToSession(const user_manager::User* added_user,

@@ -24,6 +24,7 @@
#include "chrome/test/base/testing_profile.h" #include "chrome/test/base/testing_profile.h"
#include "chromeos/ash/components/login/login_state/login_state.h" #include "chromeos/ash/components/login/login_state/login_state.h"
#include "components/user_manager/fake_user_manager.h" #include "components/user_manager/fake_user_manager.h"
#include "components/user_manager/fake_user_manager_delegate.h"
#include "components/user_manager/known_user.h" #include "components/user_manager/known_user.h"
#include "components/user_manager/multi_user/multi_user_sign_in_policy_controller.h" #include "components/user_manager/multi_user/multi_user_sign_in_policy_controller.h"
#include "components/user_manager/user.h" #include "components/user_manager/user.h"
@@ -69,6 +70,7 @@ namespace ash {
FakeChromeUserManager::FakeChromeUserManager() FakeChromeUserManager::FakeChromeUserManager()
: UserManagerBase( : UserManagerBase(
std::make_unique<user_manager::FakeUserManagerDelegate>(),
new FakeTaskRunner(), new FakeTaskRunner(),
g_browser_process ? g_browser_process->local_state() : nullptr) { g_browser_process ? g_browser_process->local_state() : nullptr) {
ProfileHelper::SetProfileToUserForTestingEnabled(true); ProfileHelper::SetProfileToUserForTestingEnabled(true);
@@ -565,11 +567,6 @@ void FakeChromeUserManager::SimulateUserProfileLoad(
} }
} }
const std::string& FakeChromeUserManager::GetApplicationLocale() const {
static const std::string default_locale("en-US");
return default_locale;
}
void FakeChromeUserManager::LoadDeviceLocalAccounts( void FakeChromeUserManager::LoadDeviceLocalAccounts(
std::set<AccountId>* users_set) { std::set<AccountId>* users_set) {
NOTREACHED(); NOTREACHED();

@@ -131,7 +131,6 @@ class FakeChromeUserManager : public user_manager::UserManagerBase {
GetMultiUserSignInPolicyController() override; GetMultiUserSignInPolicyController() override;
// user_manager::UserManagerBase override. // user_manager::UserManagerBase override.
const std::string& GetApplicationLocale() const override;
void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override; void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override;
bool IsEnterpriseManaged() const override; bool IsEnterpriseManaged() const override;
void PerformPostUserLoggedInActions(bool browser_restart) override; void PerformPostUserLoggedInActions(bool browser_restart) override;

@@ -0,0 +1,18 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/ash/login/users/user_manager_delegate_impl.h"
#include "chrome/browser/browser_process.h"
namespace ash {
UserManagerDelegateImpl::UserManagerDelegateImpl() = default;
UserManagerDelegateImpl::~UserManagerDelegateImpl() = default;
const std::string& UserManagerDelegateImpl::GetApplicationLocale() {
return g_browser_process->GetApplicationLocale();
}
} // namespace ash

@@ -0,0 +1,26 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_ASH_LOGIN_USERS_USER_MANAGER_DELEGATE_IMPL_H_
#define CHROME_BROWSER_ASH_LOGIN_USERS_USER_MANAGER_DELEGATE_IMPL_H_
#include "components/user_manager/user_manager_base.h"
namespace ash {
// Implementation of UserManagerBase::Delegate to inject //chrome/* behavior.
class UserManagerDelegateImpl : public user_manager::UserManagerBase::Delegate {
public:
UserManagerDelegateImpl();
UserManagerDelegateImpl(const UserManagerDelegateImpl&) = delete;
UserManagerDelegateImpl& operator=(const UserManagerDelegateImpl&) = delete;
~UserManagerDelegateImpl() override;
// UserManagerBase::Delegate:
const std::string& GetApplicationLocale() override;
};
} // namespace ash
#endif // CHROME_BROWSER_ASH_LOGIN_USERS_USER_MANAGER_DELEGATE_IMPL_H_

@@ -77,6 +77,8 @@ if (is_chromeos_ash) {
"fake_device_ownership_waiter.h", "fake_device_ownership_waiter.h",
"fake_user_manager.cc", "fake_user_manager.cc",
"fake_user_manager.h", "fake_user_manager.h",
"fake_user_manager_delegate.cc",
"fake_user_manager_delegate.h",
] ]
public_deps = [ public_deps = [

@@ -11,6 +11,7 @@
#include "base/ranges/algorithm.h" #include "base/ranges/algorithm.h"
#include "base/system/sys_info.h" #include "base/system/sys_info.h"
#include "base/task/single_thread_task_runner.h" #include "base/task/single_thread_task_runner.h"
#include "components/user_manager/fake_user_manager_delegate.h"
#include "components/user_manager/user_names.h" #include "components/user_manager/user_names.h"
#include "components/user_manager/user_type.h" #include "components/user_manager/user_type.h"
@@ -41,7 +42,9 @@ class FakeTaskRunner : public base::SingleThreadTaskRunner {
namespace user_manager { namespace user_manager {
FakeUserManager::FakeUserManager(PrefService* local_state) FakeUserManager::FakeUserManager(PrefService* local_state)
: UserManagerBase(new FakeTaskRunner(), local_state) {} : UserManagerBase(std::make_unique<FakeUserManagerDelegate>(),
new FakeTaskRunner(),
local_state) {}
FakeUserManager::~FakeUserManager() = default; FakeUserManager::~FakeUserManager() = default;
@@ -356,11 +359,6 @@ bool FakeUserManager::IsEphemeralAccountIdByPolicy(
return GetEphemeralModeConfig().IsAccountIdIncluded(account_id); return GetEphemeralModeConfig().IsAccountIdIncluded(account_id);
} }
const std::string& FakeUserManager::GetApplicationLocale() const {
static const std::string default_locale("en-US");
return default_locale;
}
bool FakeUserManager::IsEnterpriseManaged() const { bool FakeUserManager::IsEnterpriseManaged() const {
return false; return false;
} }

@@ -126,7 +126,6 @@ class USER_MANAGER_EXPORT FakeUserManager : public UserManagerBase {
void SetEphemeralModeConfig( void SetEphemeralModeConfig(
EphemeralModeConfig ephemeral_mode_config) override; EphemeralModeConfig ephemeral_mode_config) override;
const std::string& GetApplicationLocale() const override;
bool IsEnterpriseManaged() const override; bool IsEnterpriseManaged() const override;
void LoadDeviceLocalAccounts( void LoadDeviceLocalAccounts(
std::set<AccountId>* device_local_accounts_set) override {} std::set<AccountId>* device_local_accounts_set) override {}

@@ -0,0 +1,21 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "components/user_manager/fake_user_manager_delegate.h"
#include <string>
#include "base/no_destructor.h"
namespace user_manager {
FakeUserManagerDelegate::FakeUserManagerDelegate() = default;
FakeUserManagerDelegate::~FakeUserManagerDelegate() = default;
const std::string& FakeUserManagerDelegate::GetApplicationLocale() {
static const base::NoDestructor<std::string> default_locale("en-US");
return *default_locale;
}
} // namespace user_manager

@@ -0,0 +1,28 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef COMPONENTS_USER_MANAGER_FAKE_USER_MANAGER_DELEGATE_H_
#define COMPONENTS_USER_MANAGER_FAKE_USER_MANAGER_DELEGATE_H_
#include "components/user_manager/user_manager_base.h"
#include "components/user_manager/user_manager_export.h"
namespace user_manager {
// Fake implementation of UserManagerBase::Delegate.
class USER_MANAGER_EXPORT FakeUserManagerDelegate
: public UserManagerBase::Delegate {
public:
FakeUserManagerDelegate();
FakeUserManagerDelegate(const FakeUserManagerDelegate&) = delete;
FakeUserManagerDelegate& operator=(const FakeUserManagerDelegate&) = delete;
~FakeUserManagerDelegate() override;
// UserManagerBase::Delegate:
const std::string& GetApplicationLocale() override;
};
} // namespace user_manager
#endif // COMPONENTS_USER_MANAGER_FAKE_USER_MANAGER_DELEGATE_H_

@@ -109,9 +109,12 @@ void UserManagerBase::RegisterPrefs(PrefRegistrySimple* registry) {
} }
UserManagerBase::UserManagerBase( UserManagerBase::UserManagerBase(
std::unique_ptr<Delegate> delegate,
scoped_refptr<base::SingleThreadTaskRunner> task_runner, scoped_refptr<base::SingleThreadTaskRunner> task_runner,
PrefService* local_state) PrefService* local_state)
: task_runner_(std::move(task_runner)), local_state_(local_state) { : delegate_(std::move(delegate)),
task_runner_(std::move(task_runner)),
local_state_(local_state) {
// |local_state| can be nullptr only for testing. // |local_state| can be nullptr only for testing.
if (!local_state) { if (!local_state) {
CHECK_IS_TEST(); CHECK_IS_TEST();
@@ -1357,7 +1360,7 @@ void UserManagerBase::SendGaiaUserLoginMetrics(const AccountId& account_id) {
void UserManagerBase::UpdateUserAccountLocale(const AccountId& account_id, void UserManagerBase::UpdateUserAccountLocale(const AccountId& account_id,
const std::string& locale) { const std::string& locale) {
std::unique_ptr<std::string> resolved_locale(new std::string()); std::unique_ptr<std::string> resolved_locale(new std::string());
if (!locale.empty() && locale != GetApplicationLocale()) { if (!locale.empty() && locale != delegate_->GetApplicationLocale()) {
// std::move will nullptr out |resolved_locale|, so cache the underlying // std::move will nullptr out |resolved_locale|, so cache the underlying
// ptr. // ptr.
std::string* raw_resolved_locale = resolved_locale.get(); std::string* raw_resolved_locale = resolved_locale.get();

@@ -64,9 +64,22 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
kMaxValue = kLSUDeleted kMaxValue = kLSUDeleted
}; };
// Delegate interface to inject //chrome/* dependency.
// In case you need to extend this, please consider to minimize the
// responsibility, because it means to depend more things on //chrome/*
// browser from ash-system, which we prefer minimizing.
class Delegate {
public:
virtual ~Delegate() = default;
// Returns the application locale.
virtual const std::string& GetApplicationLocale() = 0;
};
// Creates UserManagerBase with |task_runner| for UI thread, and given // Creates UserManagerBase with |task_runner| for UI thread, and given
// |local_state|. |local_state| must outlive this UserManager. // |local_state|. |local_state| must outlive this UserManager.
UserManagerBase(scoped_refptr<base::SingleThreadTaskRunner> task_runner, UserManagerBase(std::unique_ptr<Delegate> delegate,
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
PrefService* local_state); PrefService* local_state);
UserManagerBase(const UserManagerBase&) = delete; UserManagerBase(const UserManagerBase&) = delete;
@@ -203,9 +216,6 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
// equals to active_user_, active_user_ is reset to NULL. // equals to active_user_, active_user_ is reset to NULL.
virtual void DeleteUser(User* user); virtual void DeleteUser(User* user);
// Returns the locale used by the application.
virtual const std::string& GetApplicationLocale() const = 0;
// Loads |users_| from Local State if the list has not been loaded yet. // Loads |users_| from Local State if the list has not been loaded yet.
// Subsequent calls have no effect. Must be called on the UI thread. // Subsequent calls have no effect. Must be called on the UI thread.
virtual void EnsureUsersLoaded(); virtual void EnsureUsersLoaded();
@@ -381,6 +391,8 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
void RemoveLegacySupervisedUser(const AccountId& account_id); void RemoveLegacySupervisedUser(const AccountId& account_id);
std::unique_ptr<Delegate> delegate_;
// Indicates stage of loading user from prefs. // Indicates stage of loading user from prefs.
UserLoadStage user_loading_stage_ = STAGE_NOT_LOADED; UserLoadStage user_loading_stage_ = STAGE_NOT_LOADED;