Switch to direct use of OwnerSettingsServiceChromeOS::Set() in tests.

chrome/browser/chromeos/ownership/fake_owner_settings_service.cc

Index: chrome/browser/chromeos/ownership/fake_owner_settings_service.cc
diff --git a/chrome/browser/chromeos/ownership/fake_owner_settings_service.cc b/chrome/browser/chromeos/ownership/fake_owner_settings_service.cc
index b5154475280f7b0328750b6e66fc64ebe8e09a1b..2f071120b7793c504104242a62cf32433e6bad7c 100644
--- a/chrome/browser/chromeos/ownership/fake_owner_settings_service.cc
+++ b/chrome/browser/chromeos/ownership/fake_owner_settings_service.cc
@@ -4,8 +4,18 @@
 
 #include "chrome/browser/chromeos/ownership/fake_owner_settings_service.h"
 
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "components/ownership/mock_owner_key_util.h"
+
 namespace chromeos {
 
+FakeOwnerSettingsService::FakeOwnerSettingsService(Profile* profile)
+    : OwnerSettingsServiceChromeOS(nullptr,
+                                   profile,
+                                   new ownership::MockOwnerKeyUtil()),
+      set_management_settings_result_(true) {
+}
+
 FakeOwnerSettingsService::FakeOwnerSettingsService(
     Profile* profile,
     const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util)
@@ -23,4 +33,11 @@ void FakeOwnerSettingsService::SetManagementSettings(
   callback.Run(set_management_settings_result_);
 }
 
+bool FakeOwnerSettingsService::Set(const std::string& setting,
+                                   const base::Value& value) {
+  CrosSettingsProvider* provider = CrosSettings::Get()->GetProvider(setting);
+  provider->Set(setting, value);

[Mattias Nissler (ping if slow), 2015/03/30 14:45:46]

This is not going to mix well with our goal to remove
CrosSettingsProvider::Set()

[Ivan Podogov, 2015/03/31 08:40:00]

As doesn't the direct use of CrosSettingsProvider::Set() in all the tests... I'm
not sure what better option there is.

[Mattias Nissler (ping if slow), 2015/03/31 08:44:48]

Ah, so we had our wires crossed here - I think all the tests have a
StubCrosSettingsProvider instance. We can have a Set() on that that we can call
from tests, but still remove CrosSettignsProvider::Set(). Or am I missing
something?

[Ivan Podogov, 2015/03/31 08:51:12]

Hm, this should be very well possible, although it would require
FakeOwnerSettingsService to have the StubCrosSettingsProvider instance access
here. We could either add some SetProvider() to FakeOwnerSettingsService class
(or require to pass a provider in the constructor), or just expect
CrosSettings::GetProvider() to always return a StubCrosSettingsProvider here.
What do you think works better?

[Mattias Nissler (ping if slow), 2015/03/31 08:56:03]

Why would FakeOwnerSettingsService need to write settings at all?

[Ivan Podogov, 2015/03/31 09:01:21]

For example, because KioskAppsManager (or KioskAppsHelper, which doesn't really
matter) writes settings in AddApp/RemoveApp through the OwnerSettingsService.
There may also be some other cases where OwnerSettingsService is used.
Basically, for the same reason the OwnerSettingsService itself has the write
path: shouldn't it become the default way to write settings?

[Mattias Nissler (ping if slow), 2015/03/31 09:16:09]

OwnerSettingsServiceChromeOS writes via DeviceSettingsService::Store().
CrosSettings will update its view of the world once the write has propagated
through the system. OwnerSettingsServiceChromeOS has no reason to push values
directly to CrosSettings.

For short-circuiting this in testing code, we might be able to find a way to
instantiate CrosSettings with a StubSettingsProvider and
FakeOwnerSettingsService s.t. we can pass a pointer to the former to the
latter's constructor? Anything is acceptable here IMHO, as long as we can get
rid of the write path in production code.

[Ivan Podogov, 2015/03/31 09:30:08]

Well, that's what I was asking in the first place: what is the better way to
pass StubSettingsProvider to FakeOwnerSettingsService for tests, since we don't
want to use the whole heavyweight device settings implementation like in PS#8.
If I'm going to use a TestingProfile::Builder to create all
FakeOwnerSettingsService instances anyway, then I guess I'll try passing the
provider in its constructor, that should be the easiest way.

[Mattias Nissler (ping if slow), 2015/03/31 09:36:01]

Ah, sorry for the confusion then. What you suggest sounds great!

+  return true;
+}
+
 }  // namespace chromeos