Reland "Migrate the Chrome OS device_sync service off of the mojo pref service"

This reverts commit 87fc2374.

Relanding after fixing initialization in the chrome crash-and-restore
case. The old code missed a call to OnProfileInitialized() in that
crash-and-restore case.

Instead of relying on chrome to call OnProfileInitialized()
in all startup code paths, we now observe the IdentityManager for when
the primary account information becomes available.

Original change's description:
> Revert "Migrate the Chrome OS device_sync service off of the mojo pref service"
>
> This reverts commit 671afa99.
>
> Reason for revert: Breaks proximity auth in the crash-and-restore
> login flow (e.g. if chrome crashes during the session and logs you
> back in automatically on restart). See crbug.com/1015215
>
> Original change's description:
> > Migrate the Chrome OS device_sync service off of the mojo pref service
> >
> > DeviceSyncService runs in the browser process on the UI thread. It can
> > directly use the Profile's PrefService*. This simplifies the code, and
> > will reduce the amount of work rockot@ has to do for Service Manager
> > cleanup.
> >
> > Delete the DeviceSyncService mojo interface and implementation object.
> > They primarily existed to support wiring up the mojo pref service.
> > Instead, directly connect the DeviceSyncClientImpl to the backing
> > DeviceSyncImpl.
> >
> > Explicitly initialize the DeviceSyncImpl when the user's profile is
> > ready. The old code had startup timing dependencies that assumed that
> > DeviceSyncImpl was created after IdentityManager had information about
> > the primary profile. The new code instantiates the DeviceSyncImpl
> > object earlier, and IdentityManager may not be ready yet.
> >
> > Bug: 977637, 1012941
> > Test: bots
> > Change-Id: Ia32b687404e5b68980cb3aecd3003e6f7e48ffcd
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849217
> > Commit-Queue: James Cook <jamescook@chromium.org>
> > Reviewed-by: Greg Kerr <kerrnel@chromium.org>
> > Reviewed-by: Josh Nohle <nohle@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Ken Rockot <rockot@google.com>
> > Cr-Commit-Position: refs/heads/master@{#706181}
>
> TBR=jamescook@chromium.org,sky@chromium.org,rockot@google.com,kerrnel@chromium.org,hansberry@chromium.org,nohle@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 977637, 1012941
> Change-Id: Id6cf5f43aac57415dae266a7871c08e4954fa24b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865602
> Reviewed-by: James Cook <jamescook@chromium.org>
> Commit-Queue: James Cook <jamescook@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#707000}

TBR=jamescook@chromium.org,sky@chromium.org,rockot@google.com,kerrnel@chromium.org,hansberry@chromium.org,nohle@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 977637, 1012941
Change-Id: I1fff46240f3b34a7d1276edb7e79a264150ceb73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869576Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Josh Nohle <nohle@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708744}

chrome/browser/chromeos/device_sync/device_sync_client_factory.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/device_sync/device_sync_client_factory.h"
 
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/chromeos/cryptauth/client_app_metadata_provider_service.h"
 #include "chrome/browser/chromeos/cryptauth/client_app_metadata_provider_service_factory.h"
 #include "chrome/browser/chromeos/cryptauth/gcm_device_info_provider_impl.h"
@@ -12,9 +13,7 @@
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
-#include "chrome/common/pref_names.h"
-#include "chromeos/constants/chromeos_features.h"
-#include "chromeos/services/device_sync/device_sync_service.h"
+#include "chromeos/services/device_sync/device_sync_impl.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client_impl.h"
 #include "chromeos/services/multidevice_setup/public/cpp/prefs.h"
@@ -22,12 +21,7 @@
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_service.h"
-#include "content/public/browser/browser_context.h"
-#include "mojo/public/cpp/bindings/pending_remote.h"
-#include "mojo/public/cpp/bindings/remote.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
-#include "services/preferences/public/mojom/preferences.mojom.h"
-#include "services/service_manager/public/cpp/connector.h"
 
 namespace chromeos {
 
@@ -43,24 +37,14 @@ bool IsEnrollmentAllowedByPolicy(content::BrowserContext* context) {
       Profile::FromBrowserContext(context)->GetPrefs());
 }
 
-std::unique_ptr<DeviceSyncService> CreateServiceInstanceForProfile(
-    Profile* profile,
-    mojo::Remote<chromeos::device_sync::mojom::DeviceSyncService>* remote) {
-  mojo::Remote<chromeos::device_sync::mojom::DeviceSyncServiceInitializer>
-      initializer;
-  auto service = std::make_unique<DeviceSyncService>(
+std::unique_ptr<DeviceSyncBase> CreateDeviceSyncImplForProfile(
+    Profile* profile) {
+  return DeviceSyncImpl::Factory::Get()->BuildInstance(
       IdentityManagerFactory::GetForProfile(profile),
       gcm::GCMProfileServiceFactory::GetForProfile(profile)->driver(),
-      chromeos::GcmDeviceInfoProviderImpl::GetInstance(),
+      profile->GetPrefs(), chromeos::GcmDeviceInfoProviderImpl::GetInstance(),
       chromeos::ClientAppMetadataProviderServiceFactory::GetForProfile(profile),
-      profile->GetURLLoaderFactory(), initializer.BindNewPipeAndPassReceiver());
-  mojo::PendingRemote<prefs::mojom::PrefStoreConnector> pref_store_connector;
-  content::BrowserContext::GetConnectorFor(profile)->Connect(
-      prefs::mojom::kServiceName,
-      pref_store_connector.InitWithNewPipeAndPassReceiver());
-  initializer->Initialize(remote->BindNewPipeAndPassReceiver(),
-                          std::move(pref_store_connector));
-  return service;
+      profile->GetURLLoaderFactory(), std::make_unique<base::OneShotTimer>());
 }
 
 }  // namespace
@@ -69,11 +53,16 @@ std::unique_ptr<DeviceSyncService> CreateServiceInstanceForProfile(
 class DeviceSyncClientHolder : public KeyedService {
  public:
   explicit DeviceSyncClientHolder(content::BrowserContext* context)
-      : service_(CreateServiceInstanceForProfile(
-            Profile::FromBrowserContext(context),
-            &remote_service_)),
-        device_sync_client_(DeviceSyncClientImpl::Factory::Get()->BuildInstance(
-            remote_service_.get())) {}
+      : device_sync_(CreateDeviceSyncImplForProfile(
+            Profile::FromBrowserContext(context))),
+        device_sync_client_(
+            DeviceSyncClientImpl::Factory::Get()->BuildInstance()) {
+    // Connect the client's mojo interface pointer to the implementation.
+    device_sync_->BindRequest(
+        mojo::MakeRequest(device_sync_client_->GetDeviceSyncPtr()));
+    // Finish client initialization.
+    device_sync_client_->Initialize(base::ThreadTaskRunnerHandle::Get());
+  }
 
   DeviceSyncClient* device_sync_client() { return device_sync_client_.get(); }
 
@@ -81,15 +70,11 @@ class DeviceSyncClientHolder : public KeyedService {
   // KeyedService:
   void Shutdown() override {
     device_sync_client_.reset();
-    service_.reset();
+    device_sync_->CloseAllBindings();
+    device_sync_.reset();
   }
 
-  mojo::Remote<chromeos::device_sync::mojom::DeviceSyncService> remote_service_;
-
-  // The in-process service instance. Never exposed publicly except through the
-  // DeviceSyncClient, which is isolated from the service by Mojo interfaces.
-  std::unique_ptr<chromeos::device_sync::DeviceSyncService> service_;
-
+  std::unique_ptr<DeviceSyncBase> device_sync_;
   std::unique_ptr<DeviceSyncClient> device_sync_client_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceSyncClientHolder);

chrome/browser/chromeos/tether/tether_service_unittest.cc

@@ -266,8 +266,8 @@ class FakeDeviceSyncClientImplFactory
   ~FakeDeviceSyncClientImplFactory() override = default;
 
   // chromeos::device_sync::DeviceSyncClientImpl::Factory:
-  std::unique_ptr<chromeos::device_sync::DeviceSyncClient> BuildInstance(
-      chromeos::device_sync::mojom::DeviceSyncService* service) override {
+  std::unique_ptr<chromeos::device_sync::DeviceSyncClient> BuildInstance()
+      override {
     auto fake_device_sync_client =
         std::make_unique<chromeos::device_sync::FakeDeviceSyncClient>();
     fake_device_sync_client->NotifyReady();

chrome/browser/prefs/browser_prefs.cc

@@ -311,6 +311,7 @@
 #include "chromeos/network/fast_transition_observer.h"
 #include "chromeos/network/proxy/proxy_config_handler.h"
 #include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
+#include "chromeos/services/device_sync/device_sync_impl.h"
 #include "chromeos/services/multidevice_setup/multidevice_setup_service.h"
 #include "chromeos/timezone/timezone_resolver.h"
 #include "components/arc/arc_prefs.h"
@@ -935,6 +936,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   chromeos::AccountManager::RegisterPrefs(registry);
   chromeos::assistant::prefs::RegisterProfilePrefsForBrowser(registry);
   chromeos::CupsPrintersManager::RegisterProfilePrefs(registry);
+  chromeos::device_sync::DeviceSyncImpl::RegisterProfilePrefs(registry);
   chromeos::first_run::RegisterProfilePrefs(registry);
   chromeos::file_system_provider::RegisterProfilePrefs(registry);
   chromeos::KerberosCredentialsManager::RegisterProfilePrefs(registry);

chrome/test/base/in_process_browser_test.cc

@@ -126,8 +126,7 @@ class FakeDeviceSyncImplFactory
   std::unique_ptr<chromeos::device_sync::DeviceSyncBase> BuildInstance(
       signin::IdentityManager* identity_manager,
       gcm::GCMDriver* gcm_driver,
-      mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-          pref_store_connector,
+      PrefService* profile_prefs,
       const chromeos::device_sync::GcmDeviceInfoProvider*
           gcm_device_info_provider,
       chromeos::device_sync::ClientAppMetadataProvider*

chromeos/services/device_sync/BUILD.gn

@@ -106,8 +106,6 @@ static_library("device_sync") {
     "device_sync_base.h",
     "device_sync_impl.cc",
     "device_sync_impl.h",
-    "device_sync_service.cc",
-    "device_sync_service.h",
     "device_sync_type_converters.cc",
     "device_sync_type_converters.h",
     "network_request_error.cc",
@@ -152,8 +150,8 @@ static_library("device_sync") {
     "//chromeos/services/device_sync/public/cpp",
     "//chromeos/services/device_sync/public/mojom",
     "//components/gcm_driver",
+    "//components/prefs",
     "//net",
-    "//services/preferences/public/cpp",
   ]
 
   visibility = [

chromeos/services/device_sync/DEPS

@@ -8,6 +8,5 @@ include_rules = [
   "+mojo/public/cpp",
   "+services/network/public",
   "+services/network/test",
-  "+services/preferences/public",
   "+components/cryptauth"
 ]

chromeos/services/device_sync/device_sync_base.cc

@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chromeos/services/device_sync/device_sync_base.h"
+
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "chromeos/services/device_sync/device_sync_base.h"
 
 namespace chromeos {
 

chromeos/services/device_sync/device_sync_impl.h

@@ -18,11 +18,9 @@
 #include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
 #include "chromeos/services/device_sync/remote_device_provider.h"
 #include "components/signin/public/identity_manager/account_info.h"
-#include "mojo/public/cpp/bindings/pending_remote.h"
-#include "mojo/public/cpp/bindings/remote.h"
-#include "services/preferences/public/cpp/pref_service_factory.h"
-#include "services/preferences/public/mojom/preferences.mojom.h"
+#include "components/signin/public/identity_manager/identity_manager.h"
 
+class PrefRegistrySimple;
 class PrefService;
 
 namespace base {
@@ -34,10 +32,6 @@ namespace gcm {
 class GCMDriver;
 }  // namespace gcm
 
-namespace signin {
-class IdentityManager;
-}  // namespace signin
-
 namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
@@ -56,12 +50,13 @@ class SoftwareFeatureManager;
 
 // Concrete DeviceSync implementation. When DeviceSyncImpl is constructed, it
 // starts an initialization flow with the following steps:
-// (1) Verify that the primary user is logged in with a valid account ID.
-// (2) Connect to the Prefs Service associated with that account.
+// (1) Check if the primary user is logged in with a valid account ID.
+// (2) If not, wait for IdentityManager to provide the primary account ID.
 // (3) Instantiate classes which communicate with the CryptAuth back-end.
 // (4) Check enrollment state; if not yet enrolled, enroll the device.
 // (5) When enrollment is valid, listen for device sync updates.
 class DeviceSyncImpl : public DeviceSyncBase,
+                       public signin::IdentityManager::Observer,
                        public CryptAuthEnrollmentManager::Observer,
                        public RemoteDeviceProvider::Observer {
  public:
@@ -77,8 +72,7 @@ class DeviceSyncImpl : public DeviceSyncBase,
     virtual std::unique_ptr<DeviceSyncBase> BuildInstance(
         signin::IdentityManager* identity_manager,
         gcm::GCMDriver* gcm_driver,
-        mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-            pref_store_conector,
+        PrefService* profile_prefs,
         const GcmDeviceInfoProvider* gcm_device_info_provider,
         ClientAppMetadataProvider* client_app_metadata_provider,
         scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
@@ -88,6 +82,8 @@ class DeviceSyncImpl : public DeviceSyncBase,
     static Factory* test_factory_instance_;
   };
 
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
   ~DeviceSyncImpl() override;
 
  protected:
@@ -117,27 +113,7 @@ class DeviceSyncImpl : public DeviceSyncBase,
  private:
   friend class DeviceSyncServiceTest;
 
-  enum class Status {
-    FETCHING_ACCOUNT_INFO,
-    CONNECTING_TO_USER_PREFS,
-    WAITING_FOR_ENROLLMENT,
-    READY
-  };
-
-  // Wrapper around preferences code. This class is necessary so that tests can
-  // override this functionality to use a fake PrefService rather than a real
-  // connection to the Preferences service.
-  class PrefConnectionDelegate {
-   public:
-    virtual ~PrefConnectionDelegate();
-
-    virtual scoped_refptr<PrefRegistrySimple> CreatePrefRegistry();
-    virtual void ConnectToPrefService(
-        mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-            pref_store_connector,
-        scoped_refptr<PrefRegistrySimple> pref_registry,
-        prefs::ConnectCallback callback);
-  };
+  enum class Status { FETCHING_ACCOUNT_INFO, WAITING_FOR_ENROLLMENT, READY };
 
   class PendingSetSoftwareFeatureRequest {
    public:
@@ -172,21 +148,21 @@ class DeviceSyncImpl : public DeviceSyncBase,
   DeviceSyncImpl(
       signin::IdentityManager* identity_manager,
       gcm::GCMDriver* gcm_driver,
-      mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-          pref_store_connector,
+      PrefService* profile_prefs,
       const GcmDeviceInfoProvider* gcm_device_info_provider,
       ClientAppMetadataProvider* client_app_metadata_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       base::Clock* clock,
-      std::unique_ptr<PrefConnectionDelegate> pref_connection_delegate,
       std::unique_ptr<base::OneShotTimer> timer);
 
   // DeviceSyncBase:
   void Shutdown() override;
 
+  // signin::IdentityManager::Observer:
+  void OnPrimaryAccountSet(
+      const CoreAccountInfo& primary_account_info) override;
+
   void ProcessPrimaryAccountInfo(const CoreAccountInfo& primary_account_info);
-  void ConnectToPrefStore();
-  void OnConnectedToPrefService(std::unique_ptr<PrefService> pref_service);
   void InitializeCryptAuthManagementObjects();
   void CompleteInitializationAfterSuccessfulEnrollment();
 
@@ -221,22 +197,17 @@ class DeviceSyncImpl : public DeviceSyncBase,
   void StartSetSoftwareFeatureTimer();
   void OnSetSoftwareFeatureTimerFired();
 
-  void SetPrefConnectionDelegateForTesting(
-      std::unique_ptr<PrefConnectionDelegate> pref_connection_delegate);
-
   signin::IdentityManager* identity_manager_;
   gcm::GCMDriver* gcm_driver_;
-  mojo::PendingRemote<prefs::mojom::PrefStoreConnector> pref_store_connector_;
+  PrefService* profile_prefs_;
   const GcmDeviceInfoProvider* gcm_device_info_provider_;
   ClientAppMetadataProvider* client_app_metadata_provider_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   base::Clock* clock_;
-  std::unique_ptr<PrefConnectionDelegate> pref_connection_delegate_;
   std::unique_ptr<base::OneShotTimer> set_software_feature_timer_;
 
   Status status_;
   CoreAccountInfo primary_account_info_;
-  std::unique_ptr<PrefService> pref_service_;
   base::flat_map<base::UnguessableToken,
                  std::unique_ptr<PendingSetSoftwareFeatureRequest>>
       id_to_pending_set_software_feature_request_map_;

chromeos/services/device_sync/device_sync_service.cc

-// Copyright 2018 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 "chromeos/services/device_sync/device_sync_service.h"
-
-#include "base/bind.h"
-#include "base/timer/timer.h"
-#include "chromeos/components/multidevice/logging/logging.h"
-#include "chromeos/services/device_sync/device_sync_base.h"
-#include "chromeos/services/device_sync/device_sync_impl.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
-
-namespace chromeos {
-
-namespace device_sync {
-
-DeviceSyncService::DeviceSyncService(
-    signin::IdentityManager* identity_manager,
-    gcm::GCMDriver* gcm_driver,
-    const GcmDeviceInfoProvider* gcm_device_info_provider,
-    ClientAppMetadataProvider* client_app_metadata_provider,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    mojo::PendingReceiver<mojom::DeviceSyncServiceInitializer> init_receiver)
-    : init_receiver_(this, std::move(init_receiver)),
-      identity_manager_(identity_manager),
-      gcm_driver_(gcm_driver),
-      gcm_device_info_provider_(gcm_device_info_provider),
-      client_app_metadata_provider_(client_app_metadata_provider),
-      url_loader_factory_(std::move(url_loader_factory)) {}
-
-DeviceSyncService::~DeviceSyncService() {
-  // Subclasses may hold onto message response callbacks. It's important that
-  // all bindings are closed by the time those callbacks are destroyed, or they
-  // will DCHECK.
-  if (device_sync_)
-    device_sync_->CloseAllBindings();
-}
-
-void DeviceSyncService::Initialize(
-    mojo::PendingReceiver<mojom::DeviceSyncService> receiver,
-    mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-        pref_store_connector) {
-  PA_LOG(VERBOSE) << "DeviceSyncService::Init()";
-  receiver_.Bind(std::move(receiver));
-  device_sync_ = DeviceSyncImpl::Factory::Get()->BuildInstance(
-      identity_manager_, gcm_driver_, std::move(pref_store_connector),
-      gcm_device_info_provider_, client_app_metadata_provider_,
-      url_loader_factory_, std::make_unique<base::OneShotTimer>());
-}
-
-void DeviceSyncService::BindDeviceSync(
-    mojo::PendingReceiver<mojom::DeviceSync> receiver) {
-  PA_LOG(VERBOSE) << "DeviceSyncService::BindDeviceSync()";
-  device_sync_->BindRequest(std::move(receiver));
-}
-
-}  // namespace device_sync
-
-}  // namespace chromeos

chromeos/services/device_sync/device_sync_service.h

-// Copyright 2018 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.
-
-#ifndef CHROMEOS_SERVICES_DEVICE_SYNC_DEVICE_SYNC_SERVICE_H_
-#define CHROMEOS_SERVICES_DEVICE_SYNC_DEVICE_SYNC_SERVICE_H_
-
-#include <memory>
-
-#include "base/memory/ref_counted.h"
-#include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
-#include "mojo/public/cpp/bindings/pending_receiver.h"
-#include "mojo/public/cpp/bindings/receiver.h"
-
-namespace gcm {
-class GCMDriver;
-}  // namespace gcm
-
-namespace signin {
-class IdentityManager;
-}  // namespace signin
-
-namespace network {
-class SharedURLLoaderFactory;
-}  // namespace network
-
-namespace chromeos {
-
-namespace device_sync {
-
-class ClientAppMetadataProvider;
-class DeviceSyncBase;
-class GcmDeviceInfoProvider;
-
-// Service which provides an implementation for DeviceSync. This service creates
-// one implementation and shares it among all connection requests.
-class DeviceSyncService : public mojom::DeviceSyncServiceInitializer,
-                          public mojom::DeviceSyncService {
- public:
-  DeviceSyncService(
-      signin::IdentityManager* identity_manager,
-      gcm::GCMDriver* gcm_driver,
-      const GcmDeviceInfoProvider* gcm_device_info_provider,
-      ClientAppMetadataProvider* client_app_metadata_provider,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      mojo::PendingReceiver<mojom::DeviceSyncServiceInitializer> init_receiver);
-  ~DeviceSyncService() override;
-
- private:
-  // mojom::DeviceSyncServiceInitializer:
-  void Initialize(mojo::PendingReceiver<mojom::DeviceSyncService> receiver,
-                  mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-                      pref_store_connector) override;
-
-  // mojom::DeviceSyncService:
-  void BindDeviceSync(
-      mojo::PendingReceiver<mojom::DeviceSync> receiver) override;
-
-  mojo::Receiver<mojom::DeviceSyncServiceInitializer> init_receiver_;
-  mojo::Receiver<mojom::DeviceSyncService> receiver_{this};
-
-  signin::IdentityManager* identity_manager_;
-  gcm::GCMDriver* gcm_driver_;
-  const GcmDeviceInfoProvider* gcm_device_info_provider_;
-  ClientAppMetadataProvider* client_app_metadata_provider_;
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
-
-  std::unique_ptr<DeviceSyncBase> device_sync_;
-
-  DISALLOW_COPY_AND_ASSIGN(DeviceSyncService);
-};
-
-}  // namespace device_sync
-
-}  // namespace chromeos
-
-#endif  // CHROMEOS_SERVICES_DEVICE_SYNC_DEVICE_SYNC_SERVICE_H_

chromeos/services/device_sync/public/cpp/BUILD.gn

@@ -69,6 +69,7 @@ source_set("unit_tests") {
     "//chromeos/services/device_sync",
     "//chromeos/services/device_sync:test_support",
     "//components/gcm_driver:test_support",
+    "//components/prefs:test_support",
     "//components/signin/public/identity_manager:test_support",
     "//net",
     "//services/network:test_support",

chromeos/services/device_sync/public/cpp/device_sync_client.cc

@@ -12,6 +12,10 @@ DeviceSyncClient::DeviceSyncClient() = default;
 
 DeviceSyncClient::~DeviceSyncClient() = default;
 
+mojom::DeviceSyncPtr* DeviceSyncClient::GetDeviceSyncPtr() {
+  return nullptr;
+}
+
 void DeviceSyncClient::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }

chromeos/services/device_sync/public/cpp/device_sync_client.h

@@ -10,12 +10,17 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "chromeos/components/multidevice/remote_device_ref.h"
 #include "chromeos/components/multidevice/software_feature.h"
 #include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
 
+namespace base {
+class TaskRunner;
+}  // namespace base
+
 namespace chromeos {
 
 namespace device_sync {
@@ -47,6 +52,13 @@ class DeviceSyncClient {
   DeviceSyncClient();
   virtual ~DeviceSyncClient();
 
+  // Completes initialization. Must be called after connecting the DeviceSync
+  // mojo interface pointer to the implementation.
+  virtual void Initialize(scoped_refptr<base::TaskRunner> task_runner) {}
+
+  // Returns the DeviceSync mojo interface pointer.
+  virtual mojom::DeviceSyncPtr* GetDeviceSyncPtr();
+
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 

chromeos/services/device_sync/public/cpp/device_sync_client_impl.cc

@@ -40,24 +40,23 @@ void DeviceSyncClientImpl::Factory::SetInstanceForTesting(
 
 DeviceSyncClientImpl::Factory::~Factory() = default;
 
-std::unique_ptr<DeviceSyncClient> DeviceSyncClientImpl::Factory::BuildInstance(
-    mojom::DeviceSyncService* service) {
-  return base::WrapUnique(new DeviceSyncClientImpl(service));
+std::unique_ptr<DeviceSyncClient>
+DeviceSyncClientImpl::Factory::BuildInstance() {
+  return std::make_unique<DeviceSyncClientImpl>();
 }
 
-DeviceSyncClientImpl::DeviceSyncClientImpl(mojom::DeviceSyncService* service)
-    : DeviceSyncClientImpl(service, base::ThreadTaskRunnerHandle::Get()) {}
-
-DeviceSyncClientImpl::DeviceSyncClientImpl(
-    mojom::DeviceSyncService* service,
-    scoped_refptr<base::TaskRunner> task_runner)
-    : binding_(this),
+DeviceSyncClientImpl::DeviceSyncClientImpl()
+    : observer_binding_(this),
       expiring_device_cache_(
-          std::make_unique<multidevice::ExpiringRemoteDeviceCache>()) {
-  service->BindDeviceSync(mojo::MakeRequest(&device_sync_ptr_));
+          std::make_unique<multidevice::ExpiringRemoteDeviceCache>()) {}
+
+DeviceSyncClientImpl::~DeviceSyncClientImpl() = default;
+
+void DeviceSyncClientImpl::Initialize(
+    scoped_refptr<base::TaskRunner> task_runner) {
   device_sync_ptr_->AddObserver(GenerateInterfacePtr(), base::OnceClosure());
 
-  // Delay calling these until after the constructor finishes.
+  // Delay calling these until after initialization finishes.
   task_runner->PostTask(
       FROM_HERE, base::BindOnce(&DeviceSyncClientImpl::LoadLocalDeviceMetadata,
                                 weak_ptr_factory_.GetWeakPtr()));
@@ -66,7 +65,9 @@ DeviceSyncClientImpl::DeviceSyncClientImpl(
                                        weak_ptr_factory_.GetWeakPtr()));
 }
 
-DeviceSyncClientImpl::~DeviceSyncClientImpl() = default;
+mojom::DeviceSyncPtr* DeviceSyncClientImpl::GetDeviceSyncPtr() {
+  return &device_sync_ptr_;
+}
 
 void DeviceSyncClientImpl::OnEnrollmentFinished() {
   // Before notifying observers that enrollment has finished, sync down the
@@ -244,7 +245,7 @@ void DeviceSyncClientImpl::OnFindEligibleDevicesCompleted(
 
 mojom::DeviceSyncObserverPtr DeviceSyncClientImpl::GenerateInterfacePtr() {
   mojom::DeviceSyncObserverPtr interface_ptr;
-  binding_.Bind(mojo::MakeRequest(&interface_ptr));
+  observer_binding_.Bind(mojo::MakeRequest(&interface_ptr));
   return interface_ptr;
 }
 

chromeos/services/device_sync/public/cpp/device_sync_client_impl.h

@@ -41,16 +41,18 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
     static Factory* Get();
     static void SetInstanceForTesting(Factory* test_factory);
     virtual ~Factory();
-    virtual std::unique_ptr<DeviceSyncClient> BuildInstance(
-        mojom::DeviceSyncService* service);
+    virtual std::unique_ptr<DeviceSyncClient> BuildInstance();
 
    private:
     static Factory* test_factory_;
   };
 
-  explicit DeviceSyncClientImpl(mojom::DeviceSyncService* service);
+  DeviceSyncClientImpl();
   ~DeviceSyncClientImpl() override;
 
+  void Initialize(scoped_refptr<base::TaskRunner> task_runner) override;
+  mojom::DeviceSyncPtr* GetDeviceSyncPtr() override;
+
   // DeviceSyncClient:
   void ForceEnrollmentNow(
       mojom::DeviceSync::ForceEnrollmentNowCallback callback) override;
@@ -77,9 +79,6 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
  private:
   friend class DeviceSyncClientImplTest;
 
-  DeviceSyncClientImpl(mojom::DeviceSyncService* service,
-                       scoped_refptr<base::TaskRunner> task_runner);
-
   void AttemptToBecomeReady();
 
   void LoadSyncedDevices();
@@ -100,7 +99,7 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
   void FlushForTesting();
 
   mojom::DeviceSyncPtr device_sync_ptr_;
-  mojo::Binding<mojom::DeviceSyncObserver> binding_;
+  mojo::Binding<mojom::DeviceSyncObserver> observer_binding_;
   std::unique_ptr<multidevice::ExpiringRemoteDeviceCache>
       expiring_device_cache_;
 

chromeos/services/device_sync/public/cpp/device_sync_client_impl_unittest.cc

@@ -19,12 +19,12 @@
 #include "base/test/test_simple_task_runner.h"
 #include "chromeos/components/multidevice/remote_device_test_util.h"
 #include "chromeos/services/device_sync/device_sync_impl.h"
-#include "chromeos/services/device_sync/device_sync_service.h"
 #include "chromeos/services/device_sync/fake_device_sync.h"
 #include "chromeos/services/device_sync/public/cpp/fake_client_app_metadata_provider.h"
 #include "chromeos/services/device_sync/public/cpp/fake_gcm_device_info_provider.h"
 #include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
 #include "components/gcm_driver/fake_gcm_driver.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -61,8 +61,7 @@ class FakeDeviceSyncImplFactory : public DeviceSyncImpl::Factory {
   std::unique_ptr<DeviceSyncBase> BuildInstance(
       signin::IdentityManager* identity_manager,
       gcm::GCMDriver* gcm_driver,
-      mojo::PendingRemote<prefs::mojom::PrefStoreConnector>
-          pref_store_connector,
+      PrefService* profile_prefs,
       const GcmDeviceInfoProvider* gcm_device_info_provider,
       ClientAppMetadataProvider* client_app_metadata_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
@@ -169,33 +168,27 @@ class DeviceSyncClientImplTest : public testing::Test {
               return nullptr;
             }));
 
-    mojo::Remote<mojom::DeviceSyncServiceInitializer> initializer;
-    service_ = std::make_unique<DeviceSyncService>(
+    test_pref_service_ = std::make_unique<TestingPrefServiceSimple>();
+    DeviceSyncImpl::RegisterProfilePrefs(test_pref_service_->registry());
+
+    device_sync_ = DeviceSyncImpl::Factory::Get()->BuildInstance(
         identity_test_environment_->identity_manager(), fake_gcm_driver_.get(),
-        fake_gcm_device_info_provider_.get(),
+        test_pref_service_.get(), fake_gcm_device_info_provider_.get(),
         fake_client_app_metadata_provider_.get(), shared_url_loader_factory,
-        initializer.BindNewPipeAndPassReceiver());
-
-    mojo::PendingRemote<prefs::mojom::PrefStoreConnector> pref_store_connector;
-    ignore_result(pref_store_connector.InitWithNewPipeAndPassReceiver());
-    initializer->Initialize(remote_service_.BindNewPipeAndPassReceiver(),
-                            std::move(pref_store_connector));
+        std::make_unique<base::OneShotTimer>());
 
     test_observer_ = std::make_unique<TestDeviceSyncClientObserver>();
 
-    CreateClient();
-  }
-
-  void CreateClient() {
-    // DeviceSyncClient's constructor posts two tasks to the TaskRunner. Idle
+    // DeviceSyncClient initialization posts two tasks to the TaskRunner. Idle
     // the TaskRunner so that the tasks can be run via a RunLoop later on.
     auto test_task_runner = base::MakeRefCounted<base::TestSimpleTaskRunner>();
-    client_ = base::WrapUnique(
-        new DeviceSyncClientImpl(remote_service_.get(), test_task_runner));
+    client_ = std::make_unique<DeviceSyncClientImpl>();
+    device_sync_->BindRequest(mojo::MakeRequest(client_->GetDeviceSyncPtr()));
+    client_->Initialize(test_task_runner);
     test_task_runner->RunUntilIdle();
   }
 
-  void InitializeClient(bool complete_enrollment_before_sync = true) {
+  void SetupClient(bool complete_enrollment_before_sync = true) {
     client_->AddObserver(test_observer_.get());
 
     SendPendingMojoMessages();
@@ -471,8 +464,8 @@ class DeviceSyncClientImplTest : public testing::Test {
       fake_client_app_metadata_provider_;
   FakeDeviceSync* fake_device_sync_;
   std::unique_ptr<FakeDeviceSyncImplFactory> fake_device_sync_impl_factory_;
-  std::unique_ptr<DeviceSyncService> service_;
-  mojo::Remote<mojom::DeviceSyncService> remote_service_;
+  std::unique_ptr<TestingPrefServiceSimple> test_pref_service_;
+  std::unique_ptr<DeviceSyncBase> device_sync_;
   std::unique_ptr<TestDeviceSyncClientObserver> test_observer_;
 
   std::unique_ptr<DeviceSyncClientImpl> client_;
@@ -543,7 +536,7 @@ class DeviceSyncClientImplTest : public testing::Test {
 
 TEST_F(DeviceSyncClientImplTest,
        TestCompleteInitialSyncBeforeInitialEnrollment) {
-  InitializeClient(false /* complete_enrollment_before_sync */);
+  SetupClient(false /* complete_enrollment_before_sync */);
 }
 
 TEST_F(
@@ -598,7 +591,7 @@ TEST_F(
 TEST_F(DeviceSyncClientImplTest, TestOnEnrollmentFinished) {
   EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
 
-  InitializeClient();
+  SetupClient();
 
   EXPECT_EQ(test_remote_device_list_[0].public_key,
             client_->GetLocalDeviceMetadata()->public_key());
@@ -635,7 +628,7 @@ TEST_F(DeviceSyncClientImplTest, TestOnEnrollmentFinished) {
 TEST_F(DeviceSyncClientImplTest, TestOnNewDevicesSynced) {
   EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
-  InitializeClient();
+  SetupClient();
 
   VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
       client_->GetSyncedDevices(), test_remote_device_list_);
@@ -664,31 +657,31 @@ TEST_F(DeviceSyncClientImplTest, TestOnNewDevicesSynced) {
 }
 
 TEST_F(DeviceSyncClientImplTest, TestForceEnrollmentNow_ExpectSuccess) {
-  InitializeClient();
+  SetupClient();
 
   CallForceEnrollmentNow(true /* expected_success */);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestForceEnrollmentNow_ExpectFailure) {
-  InitializeClient();
+  SetupClient();
 
   CallForceEnrollmentNow(false /* expected_success */);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestSyncNow_ExpectSuccess) {
-  InitializeClient();
+  SetupClient();
 
   CallSyncNow(true /* expected_success */);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestSyncNow_ExpectFailure) {
-  InitializeClient();
+  SetupClient();
 
   CallSyncNow(false /* expected_success */);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestGetSyncedDevices_DeviceRemovedFromCache) {
-  InitializeClient();
+  SetupClient();
 
   VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
       client_->GetSyncedDevices(), test_remote_device_list_);
@@ -711,13 +704,13 @@ TEST_F(DeviceSyncClientImplTest, TestGetSyncedDevices_DeviceRemovedFromCache) {
 }
 
 TEST_F(DeviceSyncClientImplTest, TestSetSoftwareFeatureState) {
-  InitializeClient();
+  SetupClient();
 
   CallSetSoftwareFeatureState(mojom::NetworkRequestResult::kSuccess);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_NoErrorCode) {
-  InitializeClient();
+  SetupClient();
 
   multidevice::RemoteDeviceList expected_eligible_devices(
       {test_remote_device_list_[0], test_remote_device_list_[1]});
@@ -731,7 +724,7 @@ TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_NoErrorCode) {
 }
 
 TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_ErrorCode) {
-  InitializeClient();
+  SetupClient();
 
   CallFindEligibleDevices(mojom::NetworkRequestResult::kEndpointNotFound,
                           multidevice::RemoteDeviceList(),
@@ -739,7 +732,7 @@ TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_ErrorCode) {
 }
 
 TEST_F(DeviceSyncClientImplTest, TestGetDevicesActivityStatus_NoErrorCode) {
-  InitializeClient();
+  SetupClient();
   std::vector<mojom::DeviceActivityStatusPtr> expected_activity_statuses;
   expected_activity_statuses.emplace_back(mojom::DeviceActivityStatus::New(
       "deviceid", base::Time(), cryptauthv2::ConnectivityStatus::ONLINE));
@@ -749,14 +742,14 @@ TEST_F(DeviceSyncClientImplTest, TestGetDevicesActivityStatus_NoErrorCode) {
 }
 
 TEST_F(DeviceSyncClientImplTest, TestGetDevicesActivityStatus_ErrorCode) {
-  InitializeClient();
+  SetupClient();
 
   CallGetDevicesActivityStatus(mojom::NetworkRequestResult::kEndpointNotFound,
                                base::nullopt);
 }
 
 TEST_F(DeviceSyncClientImplTest, TestGetDebugInfo) {
-  InitializeClient();
+  SetupClient();
 
   CallGetDebugInfo();
 }

chromeos/services/device_sync/public/mojom/BUILD.gn

@@ -12,7 +12,6 @@ mojom("mojom") {
   public_deps = [
     "//chromeos/components/multidevice/mojom",
     "//mojo/public/mojom/base",
-    "//services/preferences/public/mojom",
   ]
 }
 

chromeos/services/device_sync/public/mojom/device_sync.mojom

@@ -6,7 +6,6 @@ module chromeos.device_sync.mojom;
 
 import "chromeos/components/multidevice/mojom/multidevice_types.mojom";
 import "mojo/public/mojom/base/time.mojom";
-import "services/preferences/public/mojom/preferences.mojom";
 
 enum NetworkRequestResult {
   // Successful network request.
@@ -160,8 +159,8 @@ interface DeviceSync {
   // On success, this function returns a null error_code with the activity
   // statuses to the callback; on error, it returns a valid error_code string
   // indicating the reason for failure along with a null activity statuses.
-  // 
-  // This method is expected to be used by multidevice_setup service running 
+  //
+  // This method is expected to be used by multidevice_setup service running
   // in the browser process.
   GetDevicesActivityStatus() =>
           (NetworkRequestResult result_code,
@@ -174,19 +173,3 @@ interface DeviceSync {
   GetDebugInfo() => (DebugInfo? debug_info);
 };
 
-// The main interface to the Device Sync service, used to initialized a new
-// instance of the service.
-interface DeviceSyncServiceInitializer {
-  // Initializes the service instance, binding a DeviceSyncService endpoint
-  // with access to a given PrefStoreConnector.
-  Initialize(
-      pending_receiver<DeviceSyncService> receiver,
-      pending_remote<prefs.mojom.PrefStoreConnector> pref_store_connector);
-};
-
-// The main interface to the Device Sync service.
-interface DeviceSyncService {
-  // Binds a new DeviceSync endpoint.
-  BindDeviceSync(pending_receiver<DeviceSync> receiver);
-};
-