From 3ef901b2c62ba03cb658d4eaa49f7d56d1f53f07 Mon Sep 17 00:00:00 2001
From: Hidehiko Abe <hidehiko@chromium.org>
Date: Mon, 25 Mar 2024 17:27:12 +0000
Subject: [PATCH] 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}
---
chrome/browser/ash/BUILD.gn | 2 ++
.../login/users/chrome_user_manager_impl.cc | 6 ++--
.../login/users/chrome_user_manager_impl.h | 1 -
.../login/users/fake_chrome_user_manager.cc | 7 ++---
.../login/users/fake_chrome_user_manager.h | 1 -
.../login/users/user_manager_delegate_impl.cc | 18 ++++++++++++
.../login/users/user_manager_delegate_impl.h | 26 +++++++++++++++++
components/user_manager/BUILD.gn | 2 ++
components/user_manager/fake_user_manager.cc | 10 +++----
components/user_manager/fake_user_manager.h | 1 -
.../fake_user_manager_delegate.cc | 21 ++++++++++++++
.../user_manager/fake_user_manager_delegate.h | 28 +++++++++++++++++++
components/user_manager/user_manager_base.cc | 7 +++--
components/user_manager/user_manager_base.h | 20 ++++++++++---
14 files changed, 126 insertions(+), 24 deletions(-)
create mode 100644 chrome/browser/ash/login/users/user_manager_delegate_impl.cc
create mode 100644 chrome/browser/ash/login/users/user_manager_delegate_impl.h
create mode 100644 components/user_manager/fake_user_manager_delegate.cc
create mode 100644 components/user_manager/fake_user_manager_delegate.h
diff --git a/chrome/browser/ash/BUILD.gn b/chrome/browser/ash/BUILD.gn
index a5aa238a56e3a..65b89da4f01d4 100644
--- a/chrome/browser/ash/BUILD.gn
+++ b/chrome/browser/ash/BUILD.gn
@@ -2198,6 +2198,8 @@ source_set("ash") {
"login/users/default_user_image/default_user_images.h",
"login/users/test_users.cc",
"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.h",
"login/version_updater/update_time_estimator.cc",
diff --git a/chrome/browser/ash/login/users/chrome_user_manager_impl.cc b/chrome/browser/ash/login/users/chrome_user_manager_impl.cc
index c3a7617d98348..d13f248732647 100644
--- a/chrome/browser/ash/login/users/chrome_user_manager_impl.cc
+++ b/chrome/browser/ash/login/users/chrome_user_manager_impl.cc
@@ -46,6 +46,7 @@
#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/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/device_local_account.h"
#include "chrome/browser/ash/policy/external_data/handlers/crostini_ansible_playbook_external_data_handler.h"
@@ -290,6 +291,7 @@ ChromeUserManagerImpl::CreateChromeUserManager() {
ChromeUserManagerImpl::ChromeUserManagerImpl()
: UserManagerBase(
+ std::make_unique<UserManagerDelegateImpl>(),
base::SingleThreadTaskRunner::HasCurrentDefault()
? base::SingleThreadTaskRunner::GetCurrentDefault()
: nullptr,
@@ -591,10 +593,6 @@ void ChromeUserManagerImpl::OnDeviceLocalAccountsChanged() {
// handled via the kAccountsPrefDeviceLocalAccounts device setting observer.
}
-const std::string& ChromeUserManagerImpl::GetApplicationLocale() const {
- return g_browser_process->GetApplicationLocale();
-}
-
bool ChromeUserManagerImpl::IsEnterpriseManaged() const {
policy::BrowserPolicyConnectorAsh* connector =
g_browser_process->platform_part()->browser_policy_connector_ash();
diff --git a/chrome/browser/ash/login/users/chrome_user_manager_impl.h b/chrome/browser/ash/login/users/chrome_user_manager_impl.h
index 6b05858c20310..7855811dfdccf 100644
--- a/chrome/browser/ash/login/users/chrome_user_manager_impl.h
+++ b/chrome/browser/ash/login/users/chrome_user_manager_impl.h
@@ -117,7 +117,6 @@ class ChromeUserManagerImpl
void OnProfileWillBeDestroyed(Profile* profile) override;
protected:
- const std::string& GetApplicationLocale() const override;
void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override;
void NotifyOnLogin() override;
void NotifyUserAddedToSession(const user_manager::User* added_user,
diff --git a/chrome/browser/ash/login/users/fake_chrome_user_manager.cc b/chrome/browser/ash/login/users/fake_chrome_user_manager.cc
index 2c4da673e09b9..e5ad13d22082e 100644
--- a/chrome/browser/ash/login/users/fake_chrome_user_manager.cc
+++ b/chrome/browser/ash/login/users/fake_chrome_user_manager.cc
@@ -24,6 +24,7 @@
#include "chrome/test/base/testing_profile.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_delegate.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/user.h"
@@ -69,6 +70,7 @@ namespace ash {
FakeChromeUserManager::FakeChromeUserManager()
: UserManagerBase(
+ std::make_unique<user_manager::FakeUserManagerDelegate>(),
new FakeTaskRunner(),
g_browser_process ? g_browser_process->local_state() : nullptr) {
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(
std::set<AccountId>* users_set) {
NOTREACHED();
diff --git a/chrome/browser/ash/login/users/fake_chrome_user_manager.h b/chrome/browser/ash/login/users/fake_chrome_user_manager.h
index ad85716923ab3..678140d35bac5 100644
--- a/chrome/browser/ash/login/users/fake_chrome_user_manager.h
+++ b/chrome/browser/ash/login/users/fake_chrome_user_manager.h
@@ -131,7 +131,6 @@ class FakeChromeUserManager : public user_manager::UserManagerBase {
GetMultiUserSignInPolicyController() override;
// user_manager::UserManagerBase override.
- const std::string& GetApplicationLocale() const override;
void LoadDeviceLocalAccounts(std::set<AccountId>* users_set) override;
bool IsEnterpriseManaged() const override;
void PerformPostUserLoggedInActions(bool browser_restart) override;
diff --git a/chrome/browser/ash/login/users/user_manager_delegate_impl.cc b/chrome/browser/ash/login/users/user_manager_delegate_impl.cc
new file mode 100644
index 0000000000000..458750a54b6ec
--- /dev/null
+++ b/chrome/browser/ash/login/users/user_manager_delegate_impl.cc
@@ -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
diff --git a/chrome/browser/ash/login/users/user_manager_delegate_impl.h b/chrome/browser/ash/login/users/user_manager_delegate_impl.h
new file mode 100644
index 0000000000000..9ef33b6d7c664
--- /dev/null
+++ b/chrome/browser/ash/login/users/user_manager_delegate_impl.h
@@ -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_
diff --git a/components/user_manager/BUILD.gn b/components/user_manager/BUILD.gn
index b237381a1d3ae..ded1fc8ba5302 100644
--- a/components/user_manager/BUILD.gn
+++ b/components/user_manager/BUILD.gn
@@ -77,6 +77,8 @@ if (is_chromeos_ash) {
"fake_device_ownership_waiter.h",
"fake_user_manager.cc",
"fake_user_manager.h",
+ "fake_user_manager_delegate.cc",
+ "fake_user_manager_delegate.h",
]
public_deps = [
diff --git a/components/user_manager/fake_user_manager.cc b/components/user_manager/fake_user_manager.cc
index cc29a1e506a28..13f4e8089bd80 100644
--- a/components/user_manager/fake_user_manager.cc
+++ b/components/user_manager/fake_user_manager.cc
@@ -11,6 +11,7 @@
#include "base/ranges/algorithm.h"
#include "base/system/sys_info.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_type.h"
@@ -41,7 +42,9 @@ class FakeTaskRunner : public base::SingleThreadTaskRunner {
namespace user_manager {
FakeUserManager::FakeUserManager(PrefService* local_state)
- : UserManagerBase(new FakeTaskRunner(), local_state) {}
+ : UserManagerBase(std::make_unique<FakeUserManagerDelegate>(),
+ new FakeTaskRunner(),
+ local_state) {}
FakeUserManager::~FakeUserManager() = default;
@@ -356,11 +359,6 @@ bool FakeUserManager::IsEphemeralAccountIdByPolicy(
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 {
return false;
}
diff --git a/components/user_manager/fake_user_manager.h b/components/user_manager/fake_user_manager.h
index 1c4bde13abf01..55b60cd3a34a1 100644
--- a/components/user_manager/fake_user_manager.h
+++ b/components/user_manager/fake_user_manager.h
@@ -126,7 +126,6 @@ class USER_MANAGER_EXPORT FakeUserManager : public UserManagerBase {
void SetEphemeralModeConfig(
EphemeralModeConfig ephemeral_mode_config) override;
- const std::string& GetApplicationLocale() const override;
bool IsEnterpriseManaged() const override;
void LoadDeviceLocalAccounts(
std::set<AccountId>* device_local_accounts_set) override {}
diff --git a/components/user_manager/fake_user_manager_delegate.cc b/components/user_manager/fake_user_manager_delegate.cc
new file mode 100644
index 0000000000000..fe3429150f6f0
--- /dev/null
+++ b/components/user_manager/fake_user_manager_delegate.cc
@@ -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
diff --git a/components/user_manager/fake_user_manager_delegate.h b/components/user_manager/fake_user_manager_delegate.h
new file mode 100644
index 0000000000000..ea61b02f418f3
--- /dev/null
+++ b/components/user_manager/fake_user_manager_delegate.h
@@ -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_
diff --git a/components/user_manager/user_manager_base.cc b/components/user_manager/user_manager_base.cc
index 644a88aa9b8c1..fa95b425dbe50 100644
--- a/components/user_manager/user_manager_base.cc
+++ b/components/user_manager/user_manager_base.cc
@@ -109,9 +109,12 @@ void UserManagerBase::RegisterPrefs(PrefRegistrySimple* registry) {
}
UserManagerBase::UserManagerBase(
+ std::unique_ptr<Delegate> delegate,
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
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.
if (!local_state) {
CHECK_IS_TEST();
@@ -1357,7 +1360,7 @@ void UserManagerBase::SendGaiaUserLoginMetrics(const AccountId& account_id) {
void UserManagerBase::UpdateUserAccountLocale(const AccountId& account_id,
const std::string& locale) {
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
// ptr.
std::string* raw_resolved_locale = resolved_locale.get();
diff --git a/components/user_manager/user_manager_base.h b/components/user_manager/user_manager_base.h
index d7d3bcf951095..55e0b246b2917 100644
--- a/components/user_manager/user_manager_base.h
+++ b/components/user_manager/user_manager_base.h
@@ -64,9 +64,22 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
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
// |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);
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.
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.
// Subsequent calls have no effect. Must be called on the UI thread.
virtual void EnsureUsersLoaded();
@@ -381,6 +391,8 @@ class USER_MANAGER_EXPORT UserManagerBase : public UserManager {
void RemoveLegacySupervisedUser(const AccountId& account_id);
+ std::unique_ptr<Delegate> delegate_;
+
// Indicates stage of loading user from prefs.
UserLoadStage user_loading_stage_ = STAGE_NOT_LOADED;