mash: switch to the new pref service

ash/shell.cc

Index: ash/shell.cc
diff --git a/ash/shell.cc b/ash/shell.cc
index b08021413c6e2188221ca9c02c7951733b8fecbd..43361f33a7543d459479eb25eb2358a2dbae0676 100644
--- a/ash/shell.cc
+++ b/ash/shell.cc
@@ -124,8 +124,10 @@
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/system/devicemode.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 #include "components/ui_devtools/devtools_server.h"
-#include "services/preferences/public/cpp/pref_client_store.h"
+#include "services/preferences/public/cpp/pref_service_factory.h"
 #include "services/preferences/public/interfaces/preferences.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/ui/public/interfaces/constants.mojom.h"
@@ -728,7 +730,7 @@ Shell::~Shell() {
   // Must be destroyed before FocusController.
   shelf_delegate_.reset();
 
-  // Removes itself as an observer of |pref_store_|.
+  // Removes itself as an observer of |pref_service_|.
   shelf_controller_.reset();
 
   wm_shell_->Shutdown();
@@ -760,7 +762,7 @@ Shell::~Shell() {
   wm_shell_.reset();
   session_controller_->RemoveSessionStateObserver(this);
   wallpaper_delegate_.reset();
-  pref_store_ = nullptr;
+  pref_service_ = nullptr;
   shell_delegate_.reset();
 
   DCHECK(instance_ == this);
@@ -775,11 +777,12 @@ void Shell::Init(const ShellInitParams& init_params) {
   wallpaper_delegate_ = shell_delegate_->CreateWallpaperDelegate();
 
   // Can be null in tests.
-  if (shell_delegate_->GetShellConnector()) {
-    prefs::mojom::PreferencesServiceFactoryPtr pref_factory_ptr;
-    shell_delegate_->GetShellConnector()->BindInterface(
-        prefs::mojom::kServiceName, &pref_factory_ptr);
-    pref_store_ = new preferences::PrefClientStore(std::move(pref_factory_ptr));
+  if (wm_shell_->IsRunningInMash() && shell_delegate_->GetShellConnector()) {

[jonross, 2017/03/27 14:26:38]

More detailed notes given for PrefServiceEnabled(), I don't like the restriction
to mash only.

We have features being worked on which want to use the preference service both
in mash and classic ash modes.

[sky, 2017/03/27 16:00:10]

How come the pref-service is specific to mash?

[tibell, 2017/03/28 00:34:31]

I'm happy to use whatever predicate you prefer. It just needs to be possible to
evaluate in all the cases it's used in this CL (e.g. both in chrome/ and ash/).
Let me know what the right predicate is, given these constraints:

* We don't want to run the preferences_forwarder service unless needed. In a
servicified world services which want to access profile data should be running
as the user ID of the profile. What we're doing here is a workaround until
that's true everywhere.
* The pref service is behind a feature flag for the moment, just so we can make
sure there are no performance implications for Chrome before we turn it on
everywhere.

I don't think the predicate can be <always on for Chrome OS> before we make the
above feature flag on by default, which we expect to do in April. Could --mash
implying --features=PrefService be enough until then? I don't think anyone is
actually use the PrefService instance in shell.h yet.

[tibell, 2017/03/28 00:34:31]

I didn't explain this clearly enough.

The pref service itself is always running*. One instance per profile is created
by the factory registered in ProfileImpl::RegisterInProcessServices. However, we
need to deal with mash (and ash?) currently running all its services as the root
user but wanting to talk to the pref service instance running for the active
profile. The preferences_forwarder services (implemented by
ActiveProfilePrefService, using exactly the same interface as the regular pref
service) lets mash pretend it's talking to the pref service under the right user
ID.

* It's actually behind a feature flag right now, until we've validated that
performance is good enough to turn it on everywhere, but in principle it will be
always running.

See my reply to jonross@ above for more details on when which service should
run.

[jonross, 2017/03/28 13:57:26]

I hadn't realized the concerns around performance. Nor the plan to enable the
flag by default for April.

The Ash WallpaperController was actually beginning to hook up to the version
being replaced by this CL, targeting M59. Which would be ahead of the goal of
end of April, and would be predicate on the service always being available for
Chrome OS.

However if that is not feasible they could look at an alternate solution for
M59.

[tibell, 2017/03/29 02:59:14]

For the record, jonross@ and I discussed this offline. We'll go ahead and submit
this and try to get it ready in April, but even if we don't the alternative
solution mentioned above is OK.

+    prefs::ConnectToPrefService(
+        shell_delegate_->GetShellConnector(),
+        make_scoped_refptr(new PrefRegistrySimple()),
+        base::Bind(&Shell::OnPrefServiceInitialized, base::Unretained(this)),

[jonross, 2017/03/27 14:26:38]

Slightly concerned about an Unretained ref.

'Should' be fine in normal running. Tests implementing a ShellConnector might
run into issues with this if they aren't pumping the MessageLoop.

[tibell, 2017/03/28 00:34:31]

Acknowledged.

+        prefs::mojom::kForwarderServiceName);
   }
 
   // Some delegates access WmShell during their construction. Create them here
@@ -1226,4 +1229,10 @@ void Shell::SessionStateChanged(session_manager::SessionState state) {
   }
 }
 
+void Shell::OnPrefServiceInitialized(
+    std::unique_ptr<::PrefService> pref_service) {

[jonross, 2017/03/27 14:26:38]

We could protect against the base::Unretained by exiting early on a null
instance_

[tibell, 2017/03/28 00:34:31]

Done.

+  DCHECK(pref_service);
+  pref_service_ = std::move(pref_service);
+}
+
 }  // namespace ash