mash: switch to the new pref service

ash/shell.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/shell.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 106 matching lines @@
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/sys_info.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/trace_event/trace_event.h"
 #include "chromeos/audio/audio_a11y_controller.h"
 #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"
 #include "ui/app_list/presenter/app_list.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/env.h"
 #include "ui/aura/layout_manager.h"
 #include "ui/aura/mus/user_activity_forwarder.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_event_dispatcher.h"
@@ skipping 582 matching lines @@
   // ShelfWindowWatcher has window observers and a pointer to the shelf model.
   shelf_window_watcher_.reset();
 
   // ShelfItemDelegate subclasses it owns have complex cleanup to run (e.g. ARC
   // shelf items in Chrome) so explicitly shutdown early.
   shelf_model()->DestroyItemDelegates();
 
   // 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();
 
   // Depends on |focus_controller_|, so must be destroyed before.
   window_tree_host_manager_.reset();
   focus_controller_->RemoveObserver(this);
   focus_controller_.reset();
   screen_position_controller_.reset();
 
@@ skipping 11 matching lines @@
 
   PowerStatus::Shutdown();
 
   // Ensure that DBusThreadManager outlives this Shell.
   DCHECK(chromeos::DBusThreadManager::IsInitialized());
 
   // Needs to happen right before |instance_| is reset.
   wm_shell_.reset();
   session_controller_->RemoveSessionStateObserver(this);
   wallpaper_delegate_.reset();
-  pref_store_ = nullptr;
+  pref_service_ = nullptr;
   shell_delegate_.reset();
 
   DCHECK(instance_ == this);
   instance_ = nullptr;
 }
 
 void Shell::Init(const ShellInitParams& init_params) {
   const bool is_mash = wm_shell_->IsRunningInMash();
 
   blocking_pool_ = init_params.blocking_pool;
 
   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
   // instead of the WmShell constructor.
   accessibility_delegate_.reset(shell_delegate_->CreateAccessibilityDelegate());
   palette_delegate_ = shell_delegate_->CreatePaletteDelegate();
   toast_manager_ = base::MakeUnique<ToastManager>();
 
   // Install the custom factory early on so that views::FocusManagers for Tray,
   // Shelf, and WallPaper could be created by the factory.
@@ skipping 426 matching lines @@
   }
 
   // Only trigger an update in mash because with classic ash chrome calls
   // UpdateAfterLoginStatusChange() directly.
   if (wm_shell_->IsRunningInMash()) {
     // TODO(jamescook): Should this call Shell::OnLoginStatusChanged() too?
     UpdateAfterLoginStatusChange(session_controller_->GetLoginStatus());
   }
 }
 
+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