Remove 1 call to CrosSettings::Get()->Set

In ScopedCrosSettingsTestHelper, there was one call to
CrosSettings::Get()->Set - this was just used for testing to set
any CrosSetting to a particular value.

If every call to CrosSetting::Get()->Set can be removed, that
means we can delete the entire write path in DeviceSettingsProvider,
which will make code simpler. That means all writes will be using
OwnerSettingsService, which is better because it is profile-aware.

In practise, most tests are using a StubCrosSettingsProvider,
so they don't need to use the write path of DeviceSettingsService,
or the OwnerSettingsService - they can just write it directly
to the StubCrosSettingsProvider.

This test modifies ScopedCrosSettingsTestHelper so it can also
use the StubCrosSettingsProvider when it was initialized by
CrosSettings itself. It also modifies 2 tests that were not
using a StubCrosSettingsProvider, but need to call Set, so that
now they are using a StubCrosSettingsProvider.

For more background, see go/writepath

Bug: 433840
Change-Id: I85a68d6ea116061b13f3c5e5582aa0298af1a386
Reviewed-on: https://chromium-review.googlesource.com/c/1299002Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603530}

chrome/browser/chromeos/login/existing_user_controller_auto_login_unittest.cc

@@ -63,6 +63,7 @@ class ExistingUserControllerAutoLoginTest : public ::testing::Test {
         .WillRepeatedly(Return(mock_user_manager_->CreatePublicAccountUser(
             auto_login_account_id_)));
 
+    settings_helper_.ReplaceDeviceSettingsProviderWithStub();
     settings_helper_.SetFakeSessionManager();
 
     std::unique_ptr<base::DictionaryValue> account(new base::DictionaryValue);

chrome/browser/chromeos/login/webview_login_browsertest.cc

@@ -165,6 +165,7 @@ class WebviewLoginTest : public OobeBaseTest {
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(switches::kOobeSkipPostLogin);
     command_line->AppendSwitch(::switches::kUseFakeDeviceForMediaStream);
+    command_line->AppendSwitch(switches::kStubCrosSettings);
     OobeBaseTest::SetUpCommandLine(command_line);
   }
 

chrome/browser/chromeos/settings/cros_settings.cc

@@ -68,9 +68,14 @@ CrosSettings::CrosSettings(DeviceSettingsService* device_settings_service,
       base::Bind(&CrosSettings::FireObservers,
                  // This is safe since |this| is never deleted.
                  base::Unretained(this)));
+
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kStubCrosSettings)) {
-    AddSettingsProvider(std::make_unique<StubCrosSettingsProvider>(notify_cb));
+    std::unique_ptr<StubCrosSettingsProvider> stubbed_provider =
+        std::make_unique<StubCrosSettingsProvider>(notify_cb);
+    stubbed_provider_ptr_ = stubbed_provider.get();
+    AddSettingsProvider(std::move(stubbed_provider));
+
   } else {
     AddSettingsProvider(std::make_unique<DeviceSettingsProvider>(
         notify_cb, device_settings_service, local_state));

chrome/browser/chromeos/settings/cros_settings.h

@@ -28,6 +28,7 @@ class Value;
 namespace chromeos {
 
 class DeviceSettingsService;
+class StubCrosSettingsProvider;
 
 // This class manages per-device/global settings.
 class CrosSettings {
@@ -129,6 +130,12 @@ class CrosSettings {
   // Returns the provider that handles settings with the |path| or prefix.
   CrosSettingsProvider* GetProvider(const std::string& path) const;
 
+  // Returns the StubCrosSettingsProvider. Returns |nullptr| unless the
+  // kStubCrosSettings switch is set, which is only true during testing.
+  StubCrosSettingsProvider* stubbed_provider_for_test() const {
+    return stubbed_provider_ptr_;
+  }
+
  private:
   friend class CrosSettingsTest;
 
@@ -138,6 +145,9 @@ class CrosSettings {
   // List of ChromeOS system settings providers.
   std::vector<std::unique_ptr<CrosSettingsProvider>> providers_;
 
+  // A stubbed provider - only used if the kStubCrosSettings switch is set.
+  StubCrosSettingsProvider* stubbed_provider_ptr_ = nullptr;
+
   // A map from settings names to a list of observers. Observers get fired in
   // the order they are added.
   std::map<std::string, std::unique_ptr<base::CallbackList<void(void)>>>

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.cc

@@ -41,10 +41,15 @@ ScopedCrosSettingsTestHelper::CreateOwnerSettingsService(Profile* profile) {
 }
 
 void ScopedCrosSettingsTestHelper::ReplaceDeviceSettingsProviderWithStub() {
-  CHECK(!real_settings_provider_);
-  // Swap out the DeviceSettingsProvider with our settings provider.
+  CHECK(CrosSettings::IsInitialized());
   CrosSettings* const cros_settings = CrosSettings::Get();
 
+  // If CrosSettings is already using a stub, then we shouldn't be replacing it
+  // with a different stub - that would be confusing and unnecessary.
+  CHECK(!cros_settings->stubbed_provider_for_test());
+  // And, this function shouldn't be called twice either, for the same reason:
+  CHECK(!real_settings_provider_);
+
   // TODO(olsen): This could be simplified if DeviceSettings and CrosSettings
   // were the same thing, which they nearly are, except for 3 timezone settings.
   CrosSettingsProvider* real_settings_provider =
@@ -68,31 +73,30 @@ void ScopedCrosSettingsTestHelper::RestoreRealDeviceSettingsProvider() {
   real_settings_provider_.reset();
 }
 
-bool ScopedCrosSettingsTestHelper::IsDeviceSettingsProviderStubbed() {
-  return !!real_settings_provider_;
+StubCrosSettingsProvider* ScopedCrosSettingsTestHelper::GetStubbedProvider() {
+  // If CrosSettings was already initialized with kStubCrosSettings, then
+  // we use the StubCrosSettingsProvider that was already initialized:
+  if (CrosSettings::IsInitialized() &&
+      CrosSettings::Get()->stubbed_provider_for_test()) {
+    return CrosSettings::Get()->stubbed_provider_for_test();
+  }
+  // Otherwise, we use this one - it has to be explicitly swapped in using
+  // ReplaceDeviceSettingsProviderWithStub() however.
+  return stub_settings_provider_ptr_;
 }
 
 void ScopedCrosSettingsTestHelper::SetTrustedStatus(
     CrosSettingsProvider::TrustedStatus status) {
-  stub_settings_provider_ptr_->SetTrustedStatus(status);
+  GetStubbedProvider()->SetTrustedStatus(status);
 }
 
 void ScopedCrosSettingsTestHelper::SetCurrentUserIsOwner(bool owner) {
-  stub_settings_provider_ptr_->SetCurrentUserIsOwner(owner);
+  GetStubbedProvider()->SetCurrentUserIsOwner(owner);
 }
 
 void ScopedCrosSettingsTestHelper::Set(const std::string& path,
                                        const base::Value& in_value) {
-  CHECK(DeviceSettingsProvider::IsDeviceSetting(path));
-  if (IsDeviceSettingsProviderStubbed()) {
-    // Set in the stub settings provider.
-    stub_settings_provider_ptr_->Set(path, in_value);
-  } else {
-    // Set in the real settings provider.
-    // TODO(olsen): Separate the write path (OwnerSettingsService) away from the
-    // read path (CrosSettings) - http://crbug.com/433840
-    CrosSettings::Get()->Set(path, in_value);
-  }
+  GetStubbedProvider()->Set(path, in_value);
 }
 
 void ScopedCrosSettingsTestHelper::SetBoolean(const std::string& path,

chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h

@@ -46,7 +46,20 @@ class ScopedCrosSettingsTestHelper {
   std::unique_ptr<FakeOwnerSettingsService> CreateOwnerSettingsService(
       Profile* profile);
 
-  // These methods simply call the according |stub_settings_provider_| method.
+  // Returns the stubbed CrosSettingsProvider - either the one that was
+  // initialized by |CrosSettings::Initialize()| (that is, if the switch
+  // |kStubCrosSettings| is set). Or, if CrosSettings was not initialized with
+  // a stub, this returns a stub that is only swapped into |CrosSettings| once
+  // |ReplaceDeviceSettingsProviderWithStub()| is called.
+  // Note that if you want to test the real DeviceSettingsProvider in your test
+  // (not a stub), you should set the settings using the OwnerSettingsService
+  // which uses the current user's private key to sign the settings.
+  StubCrosSettingsProvider* GetStubbedProvider();
+
+  // These methods simply call the appropriate method on |GetStubbedProvider()|.
+  // So if you use them, you need to make sure that a stubbed provider is used
+  // in your test - either by setting |kStubCrosSettings| switch or by calling
+  // |ReplaceDeviceSettingsProviderWithStub()|.
   void SetTrustedStatus(CrosSettingsProvider::TrustedStatus status);
   void SetCurrentUserIsOwner(bool owner);
   void Set(const std::string& path, const base::Value& in_value);