Remove BrowserProcess dep in c/b/chromeos/settings

DeviceSettingsProvider in chrome/browser/chromeos/settings has an
undesirable dependency on g_browser_process, which it uses to access
the LocalState singleton. This CL removes the dependency on
g_browser_process by passing in LocalState explicitly.

Note that the tests and test helpers still depend on BrowserProcess
or TestingBrowserProcess, which they use to initialize and access the
LocalState. For tests in the settings package, I need to find a way to
remove this dependency in a follow up CL, so that the settings package
along with tests & test helpers can be moved out of chrome/browser.

See go/cros-untangle2

Bug: 446937
Change-Id: If9b14940f05ceef1fa8f33e612df22d3d9173d8f
Reviewed-on: https://chromium-review.googlesource.com/1233711Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593199}

chrome/browser/chrome_browser_main.cc

@@ -1219,7 +1219,7 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   PrefService* local_state = browser_process_->local_state();
 
 #if defined(OS_CHROMEOS)
-  chromeos::CrosSettings::Initialize();
+  chromeos::CrosSettings::Initialize(local_state);
 #endif  // defined(OS_CHROMEOS)
 
   {

chrome/browser/chromeos/arc/arc_util_unittest.cc

@@ -179,9 +179,6 @@ class ChromeArcUtilTest : public testing::Test {
     user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
         std::make_unique<FakeUserManagerWithLocalState>(
             profile_manager_.get()));
-    // Used by FakeChromeUserManager.
-    chromeos::DeviceSettingsService::Initialize();
-    chromeos::CrosSettings::Initialize();
 
     profile_ = profile_manager_->CreateTestingProfile(kTestProfileName);
   }
@@ -191,8 +188,6 @@ class ChromeArcUtilTest : public testing::Test {
     ResetArcAllowedCheckForTesting(profile_);
     profile_manager_->DeleteTestingProfile(kTestProfileName);
     profile_ = nullptr;
-    chromeos::CrosSettings::Shutdown();
-    chromeos::DeviceSettingsService::Shutdown();
     user_manager_enabler_.reset();
     profile_manager_.reset();
     command_line_.reset();
@@ -215,7 +210,7 @@ class ChromeArcUtilTest : public testing::Test {
  private:
   std::unique_ptr<base::test::ScopedCommandLine> command_line_;
   content::TestBrowserThreadBundle thread_bundle_;
-  chromeos::ScopedStubInstallAttributes test_install_attributes_;
+  chromeos::ScopedCrosSettingsTestHelper cros_settings_test_helper_;
   base::ScopedTempDir data_dir_;
   std::unique_ptr<TestingProfileManager> profile_manager_;
   std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;

chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc

@@ -140,7 +140,7 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
   // Required by ExistingUserController:
   ScopedStubInstallAttributes test_install_attributes_;
   ScopedDeviceSettingsTestHelper device_settings_test_helper_;
-  ScopedTestCrosSettings test_cros_settings_;
+  ScopedTestCrosSettings test_cros_settings_{local_state_.Get()};
   MockUserManager* mock_user_manager_;
   user_manager::ScopedUserManager scoped_user_manager_;
   std::unique_ptr<ArcKioskAppManager> arc_kiosk_app_manager_;

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc

@@ -98,8 +98,9 @@ class OwnerSettingsServiceChromeOSTest : public DeviceSettingsTestBase {
 
   void SetUp() override {
     DeviceSettingsTestBase::SetUp();
-    provider_.reset(new DeviceSettingsProvider(base::Bind(&OnPrefChanged),
-                                               &device_settings_service_));
+    provider_.reset(new DeviceSettingsProvider(
+        base::Bind(&OnPrefChanged), &device_settings_service_,
+        TestingBrowserProcess::GetGlobal()->local_state()));
     owner_key_util_->SetPrivateKey(device_policy_.GetSigningKey());
     InitOwner(AccountId::FromUserEmail(device_policy_.policy_data().username()),
               true);
@@ -199,8 +200,9 @@ class OwnerSettingsServiceChromeOSNoOwnerTest
 
   void SetUp() override {
     DeviceSettingsTestBase::SetUp();
-    provider_.reset(new DeviceSettingsProvider(base::Bind(&OnPrefChanged),
-                                               &device_settings_service_));
+    provider_.reset(new DeviceSettingsProvider(
+        base::Bind(&OnPrefChanged), &device_settings_service_,
+        TestingBrowserProcess::GetGlobal()->local_state()));
     FlushDeviceSettings();
     service_ = OwnerSettingsServiceChromeOSFactory::GetForBrowserContext(
         profile_.get());

chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc

@@ -183,8 +183,9 @@ void CloudExternalDataPolicyObserverTest::SetUp() {
   shared_url_loader_factory_ =
       base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
           &url_loader_factory_);
-  cros_settings_ =
-      std::make_unique<chromeos::CrosSettings>(&device_settings_service_);
+  cros_settings_ = std::make_unique<chromeos::CrosSettings>(
+      &device_settings_service_,
+      TestingBrowserProcess::GetGlobal()->local_state());
   device_local_account_policy_service_.reset(
       new DeviceLocalAccountPolicyService(
           &session_manager_client_, &device_settings_service_,

chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/power_policy_controller.h"
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
@@ -133,8 +134,9 @@ DeviceLocalAccountPolicyServiceTestBase::
       account_2_user_id_(GenerateDeviceLocalAccountUserId(
           kAccount2,
           DeviceLocalAccount::TYPE_PUBLIC_SESSION)),
-      cros_settings_(
-          std::make_unique<chromeos::CrosSettings>(&device_settings_service_)),
+      cros_settings_(std::make_unique<chromeos::CrosSettings>(
+          &device_settings_service_,
+          TestingBrowserProcess::GetGlobal()->local_state())),
       extension_cache_task_runner_(new base::TestSimpleTaskRunner) {
   expected_policy_map_.Set(key::kSearchSuggestEnabled, POLICY_LEVEL_MANDATORY,
                            POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,

chrome/browser/chromeos/policy/device_status_collector_browsertest.cc

@@ -304,11 +304,7 @@ namespace policy {
 class DeviceStatusCollectorTest : public testing::Test {
  public:
   DeviceStatusCollectorTest()
-      : install_attributes_(
-            chromeos::StubInstallAttributes::CreateCloudManaged("managed.com",
-                                                                "device_id")),
-        settings_helper_(false),
-        user_manager_(new chromeos::MockUserManager()),
+      : user_manager_(new chromeos::MockUserManager()),
         user_manager_enabler_(base::WrapUnique(user_manager_)),
         got_session_status_(false),
         fake_kiosk_device_local_account_(
@@ -324,6 +320,8 @@ class DeviceStatusCollectorTest : public testing::Test {
                                              kArcKioskAccountId),
         user_data_dir_override_(chrome::DIR_USER_DATA),
         update_engine_client_(new chromeos::FakeUpdateEngineClient) {
+    settings_helper_.InstallAttributes()->SetCloudManaged("managed.com",
+                                                          "device_id");
     EXPECT_CALL(*user_manager_, Shutdown()).Times(1);
 
     // Although this is really a unit test which runs in the browser_tests
@@ -572,11 +570,8 @@ class DeviceStatusCollectorTest : public testing::Test {
 
   ChromeContentClient content_client_;
   ChromeContentBrowserClient browser_content_client_;
-  chromeos::ScopedStubInstallAttributes install_attributes_;
   chromeos::system::ScopedFakeStatisticsProvider fake_statistics_provider_;
   DiskMountManager::MountPointMap mount_point_map_;
-  chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
-  chromeos::ScopedTestCrosSettings test_cros_settings_;
   chromeos::ScopedCrosSettingsTestHelper settings_helper_;
   // Only set after MockRunningKioskApp or MockTODO was called.
   std::unique_ptr<chromeos::FakeOwnerSettingsService> owner_settings_service_;

chrome/browser/chromeos/settings/cros_settings.cc

@@ -24,9 +24,9 @@ namespace chromeos {
 static CrosSettings* g_cros_settings = nullptr;
 
 // static
-void CrosSettings::Initialize() {
+void CrosSettings::Initialize(PrefService* local_state) {
   CHECK(!g_cros_settings);
-  g_cros_settings = new CrosSettings(DeviceSettingsService::Get());
+  g_cros_settings = new CrosSettings(DeviceSettingsService::Get(), local_state);
 }
 
 // static
@@ -62,7 +62,8 @@ bool CrosSettings::IsUserWhitelisted(const std::string& username,
   return FindEmailInList(kAccountsPrefUsers, username, wildcard_match);
 }
 
-CrosSettings::CrosSettings(DeviceSettingsService* device_settings_service) {
+CrosSettings::CrosSettings(DeviceSettingsService* device_settings_service,
+                           PrefService* local_state) {
   CrosSettingsProvider::NotifyObserversCallback notify_cb(
       base::Bind(&CrosSettings::FireObservers,
                  // This is safe since |this| is never deleted.
@@ -72,7 +73,7 @@ CrosSettings::CrosSettings(DeviceSettingsService* device_settings_service) {
     AddSettingsProvider(std::make_unique<StubCrosSettingsProvider>(notify_cb));
   } else {
     AddSettingsProvider(std::make_unique<DeviceSettingsProvider>(
-        notify_cb, device_settings_service));
+        notify_cb, device_settings_service, local_state));
   }
   // System settings are not mocked currently.
   AddSettingsProvider(std::make_unique<SystemSettingsProvider>(notify_cb));
@@ -354,8 +355,8 @@ void CrosSettings::FireObservers(const std::string& path) {
   observer_iterator->second->Notify();
 }
 
-ScopedTestCrosSettings::ScopedTestCrosSettings() {
-  CrosSettings::Initialize();
+ScopedTestCrosSettings::ScopedTestCrosSettings(PrefService* local_state) {
+  CrosSettings::Initialize(local_state);
 }
 
 ScopedTestCrosSettings::~ScopedTestCrosSettings() {

chrome/browser/chromeos/settings/cros_settings.h

@@ -17,6 +17,8 @@
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
 
+class PrefService;
+
 namespace base {
 class DictionaryValue;
 class ListValue;
@@ -31,7 +33,7 @@ class DeviceSettingsService;
 class CrosSettings {
  public:
   // Manage singleton instance.
-  static void Initialize();
+  static void Initialize(PrefService* local_state);
   static bool IsInitialized();
   static void Shutdown();
   static CrosSettings* Get();
@@ -44,7 +46,8 @@ class CrosSettings {
 
   // Creates a device settings service instance. This is meant for unit tests,
   // production code uses the singleton returned by Get() above.
-  explicit CrosSettings(DeviceSettingsService* device_settings_service);
+  CrosSettings(DeviceSettingsService* device_settings_service,
+               PrefService* local_state);
   virtual ~CrosSettings();
 
   // Helper function to test if the given |path| is a valid cros setting.
@@ -149,7 +152,7 @@ class CrosSettings {
 // construction and tears it down again on destruction.
 class ScopedTestCrosSettings {
  public:
-  ScopedTestCrosSettings();
+  explicit ScopedTestCrosSettings(PrefService* local_state);
   ~ScopedTestCrosSettings();
 
  private:

chrome/browser/chromeos/settings/cros_settings_unittest.cc

@@ -32,7 +32,7 @@ class CrosSettingsTest : public testing::Test {
  protected:
   CrosSettingsTest()
       : local_state_(TestingBrowserProcess::GetGlobal()),
-        settings_(DeviceSettingsService::Get()),
+        settings_(DeviceSettingsService::Get(), local_state_.Get()),
         weak_factory_(this) {}
 
   ~CrosSettingsTest() override {}

chrome/browser/chromeos/settings/device_settings_provider.cc

@@ -20,7 +20,6 @@
 #include "base/syslog_logging.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/values.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/policy/device_policy_decoder_chromeos.h"
@@ -714,9 +713,11 @@ void DecodeDeviceState(const em::PolicyData& policy_data,
 
 DeviceSettingsProvider::DeviceSettingsProvider(
     const NotifyObserversCallback& notify_cb,
-    DeviceSettingsService* device_settings_service)
+    DeviceSettingsService* device_settings_service,
+    PrefService* local_state)
     : CrosSettingsProvider(notify_cb),
       device_settings_service_(device_settings_service),
+      local_state_(local_state),
       trusted_status_(TEMPORARILY_UNTRUSTED),
       ownership_status_(device_settings_service_->GetOwnershipStatus()),
       store_callback_factory_(this) {
@@ -790,7 +791,7 @@ void DeviceSettingsProvider::DoSet(const std::string& path,
     // Set the cache to the updated value.
     UpdateValuesCache(data, device_settings_, TEMPORARILY_UNTRUSTED);
 
-    if (!device_settings_cache::Store(data, g_browser_process->local_state())) {
+    if (!device_settings_cache::Store(data, local_state_)) {
       LOG(ERROR) << "Couldn't store to the temp storage.";
       NotifyObservers(path);
       return;
@@ -856,8 +857,7 @@ void DeviceSettingsProvider::OnTentativeChangesInPolicy(
 
 void DeviceSettingsProvider::RetrieveCachedData() {
   em::PolicyData policy_data;
-  if (!device_settings_cache::Retrieve(&policy_data,
-                                       g_browser_process->local_state()) ||
+  if (!device_settings_cache::Retrieve(&policy_data, local_state_) ||
       !device_settings_.ParseFromString(policy_data.policy_value())) {
     VLOG(1) << "Can't retrieve temp store, possibly not created yet.";
   }
@@ -989,8 +989,7 @@ bool DeviceSettingsProvider::UpdateFromService() {
       const em::ChromeDeviceSettingsProto* device_settings =
           device_settings_service_->device_settings();
       if (policy_data && device_settings) {
-        if (!device_settings_cache::Store(*policy_data,
-                                          g_browser_process->local_state())) {
+        if (!device_settings_cache::Store(*policy_data, local_state_)) {
           LOG(ERROR) << "Couldn't update the local state cache.";
         }
         UpdateValuesCache(*policy_data, *device_settings, TRUSTED);

chrome/browser/chromeos/settings/device_settings_provider.h

@@ -19,6 +19,8 @@
 #include "components/policy/proto/chrome_device_policy.pb.h"
 #include "components/prefs/pref_value_map.h"
 
+class PrefService;
+
 namespace base {
 class Value;
 }  // namespace base
@@ -44,7 +46,8 @@ class DeviceSettingsProvider
   typedef base::Callback<policy::DeviceMode(void)> GetDeviceModeCallback;
 
   DeviceSettingsProvider(const NotifyObserversCallback& notify_cb,
-                         DeviceSettingsService* device_settings_service);
+                         DeviceSettingsService* device_settings_service,
+                         PrefService* pref_service);
   ~DeviceSettingsProvider() override;
 
   // Returns true if |path| is handled by this provider.
@@ -108,6 +111,8 @@ class DeviceSettingsProvider
   std::vector<base::Closure> callbacks_;
 
   DeviceSettingsService* device_settings_service_;
+  PrefService* local_state_;
+
   mutable PrefValueMap migration_values_;
 
   TrustedStatus trusted_status_;

chrome/browser/chromeos/settings/device_settings_provider_unittest.cc

@@ -69,11 +69,10 @@ class DeviceSettingsProviderTest : public DeviceSettingsTestBase {
     DeviceSettingsTestBase::SetUp();
 
     EXPECT_CALL(*this, SettingChanged(_)).Times(AnyNumber());
-    provider_.reset(
-        new DeviceSettingsProvider(
-            base::Bind(&DeviceSettingsProviderTest::SettingChanged,
-                       base::Unretained(this)),
-            &device_settings_service_));
+    provider_.reset(new DeviceSettingsProvider(
+        base::Bind(&DeviceSettingsProviderTest::SettingChanged,
+                   base::Unretained(this)),
+        &device_settings_service_, local_state_.Get()));
     Mock::VerifyAndClearExpectations(this);
   }
 

chrome/browser/chromeos/settings/owner_flags_storage.cc

@@ -7,7 +7,6 @@
 #include "base/command_line.h"
 #include "base/values.h"
 #include "chrome/browser/about_flags.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/flags_ui/flags_ui_pref_names.h"

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc

@@ -132,7 +132,8 @@ void ScopedCrosSettingsTestHelper::Initialize(bool create_settings_service) {
     test_install_attributes_.reset(new ScopedStubInstallAttributes());
     CHECK(!DeviceSettingsService::IsInitialized());
     test_device_settings_service_.reset(new ScopedTestDeviceSettingsService());
-    test_cros_settings_.reset(new ScopedTestCrosSettings());
+    test_cros_settings_.reset(
+        new ScopedTestCrosSettings(g_browser_process->local_state()));
   }
 }
 

chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc

@@ -267,7 +267,8 @@ class MultiUserWindowManagerChromeOSTest : public AshTestBase {
 
 void MultiUserWindowManagerChromeOSTest::SetUp() {
   chromeos::DeviceSettingsService::Initialize();
-  chromeos::CrosSettings::Initialize();
+  chromeos::CrosSettings::Initialize(
+      TestingBrowserProcess::GetGlobal()->local_state());
   ash_test_helper()->set_test_shell_delegate(new TestShellDelegateChromeOS);
   AshTestBase::SetUp();
   profile_manager_.reset(