[sync] Remove GetSyncableServiceForType() from SyncClient

This historic mechanism to inject dependencies into ProfileSyncService
doesn't belong in SyncClient (dependencies to core sync logic / engine
itself), and it's questionable whether it should remain in this form in
BrowserSyncClient, where feature-specific dependencies are injected, and
where multiple similar approaches coexist.

Arguably, the most elegant and universal way to inject a dependency
to a sync controller is to have full access to the KeyedService that
owns the local model. Most controllers are simple and don't actually
need this: all they need is a SyncableService or
ModelTypeControllerDelegate. However, that introduces code
inconsistencies that can be confusing.

In this patch, the approach is unified for the remaining
SyncableService types, after migrating history delete directives and
preferences). The corresponding keyed services are directly injected via
BrowserSyncClient and hence SyncClient::GetSyncableServiceForType() can
be deleted.

Change-Id: I8e80482e8657624dc7c1e15b6bcf0409ae4d1336
Bug: 915154
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463617Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816550}

chrome/browser/sync/chrome_sync_client.cc

@@ -65,6 +65,7 @@
 #include "components/invalidation/impl/invalidation_switches.h"
 #include "components/invalidation/impl/profile_invalidation_provider.h"
 #include "components/password_manager/core/browser/password_store.h"
+#include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/send_tab_to_self/send_tab_to_self_sync_service.h"
 #include "components/spellcheck/spellcheck_buildflags.h"
@@ -310,6 +311,11 @@ ChromeSyncClient::GetSendTabToSelfSyncService() {
   return SendTabToSelfSyncServiceFactory::GetForProfile(profile_);
 }
 
+sync_preferences::PrefServiceSyncable*
+ChromeSyncClient::GetPrefServiceSyncable() {
+  return PrefServiceSyncableFromProfile(profile_);
+}
+
 sync_sessions::SessionSyncService* ChromeSyncClient::GetSessionSyncService() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return SessionSyncServiceFactory::GetForProfile(profile_);
@@ -568,14 +574,6 @@ ChromeSyncClient::GetExtensionsActivity() {
 base::WeakPtr<syncer::SyncableService>
 ChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
   switch (type) {
-    case syncer::PREFERENCES:
-      return PrefServiceSyncableFromProfile(profile_)
-          ->GetSyncableService(syncer::PREFERENCES)
-          ->AsWeakPtr();
-    case syncer::PRIORITY_PREFERENCES:
-      return PrefServiceSyncableFromProfile(profile_)
-          ->GetSyncableService(syncer::PRIORITY_PREFERENCES)
-          ->AsWeakPtr();
     case syncer::SEARCH_ENGINES:
       return GetWeakPtrOrNull(
           TemplateURLServiceFactory::GetForProfile(profile_));
@@ -594,10 +592,6 @@ ChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
       return ThemeServiceFactory::GetForProfile(profile_)->
           GetThemeSyncableService()->AsWeakPtr();
 #endif  // !defined(OS_ANDROID)
-    case syncer::HISTORY_DELETE_DIRECTIVES: {
-      history::HistoryService* history = GetHistoryService();
-      return history ? history->GetDeleteDirectivesSyncableService() : nullptr;
-    }
 #if BUILDFLAG(ENABLE_SPELLCHECK)
     case syncer::DICTIONARY: {
       SpellcheckService* spellcheck_service =

chrome/browser/sync/chrome_sync_client.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "chrome/browser/sync/glue/extensions_activity_monitor.h"
 #include "components/browser_sync/browser_sync_client.h"
@@ -19,16 +20,17 @@ class Profile;
 
 namespace autofill {
 class AutofillWebDataService;
-}
+}  // namespace autofill
 
 namespace password_manager {
 class PasswordStore;
-}
+}  // namespace password_manager
 
 namespace syncer {
 class ModelTypeController;
 class SyncService;
-}
+class SyncableService;
+}  // namespace syncer
 
 namespace browser_sync {
 
@@ -51,6 +53,7 @@ class ChromeSyncClient : public browser_sync::BrowserSyncClient {
   send_tab_to_self::SendTabToSelfSyncService* GetSendTabToSelfSyncService()
       override;
   sync_sessions::SessionSyncService* GetSessionSyncService() override;
+  sync_preferences::PrefServiceSyncable* GetPrefServiceSyncable() override;
   base::Closure GetPasswordStateChangedCallback() override;
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
       syncer::SyncService* sync_service) override;
@@ -59,8 +62,6 @@ class ChromeSyncClient : public browser_sync::BrowserSyncClient {
   syncer::SyncInvalidationsService* GetSyncInvalidationsService() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;
-  base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
-      syncer::ModelType type) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate>
   GetControllerDelegateForModelType(syncer::ModelType type) override;
   scoped_refptr<syncer::ModelSafeWorker> CreateModelWorkerForGroup(
@@ -69,6 +70,10 @@ class ChromeSyncClient : public browser_sync::BrowserSyncClient {
   syncer::SyncTypePreferenceProvider* GetPreferenceProvider() override;
 
  private:
+  // Convenience function used during controller creation.
+  base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
+      syncer::ModelType type);
+
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   // Creates the ModelTypeController for syncer::APPS.
   std::unique_ptr<syncer::ModelTypeController> CreateAppsModelTypeController(

components/browser_sync/BUILD.gn

@@ -29,6 +29,7 @@ static_library("browser_sync") {
     "//components/send_tab_to_self",
     "//components/sync/invalidations",
     "//components/sync_bookmarks",
+    "//components/sync_preferences",
     "//components/sync_sessions",
     "//components/sync_user_events",
     "//components/version_info",
@@ -60,10 +61,6 @@ source_set("unit_tests") {
     "//components/sync:test_support",
     "//components/sync_bookmarks",
     "//components/sync_device_info",
-    "//components/sync_preferences",
-    "//components/sync_preferences:test_support",
-    "//components/sync_sessions",
-    "//components/sync_sessions:test_support",
     "//components/version_info",
     "//components/version_info:generate_version_info",
     "//components/webdata/common",

components/browser_sync/browser_sync_client.h

@@ -30,6 +30,10 @@ namespace send_tab_to_self {
 class SendTabToSelfSyncService;
 }  // namespace send_tab_to_self
 
+namespace sync_preferences {
+class PrefServiceSyncable;
+}  // namespace sync_preferences
+
 namespace sync_sessions {
 class SessionSyncService;
 }  // namespace sync_sessions
@@ -63,6 +67,7 @@ class BrowserSyncClient : public syncer::SyncClient {
   virtual bookmarks::BookmarkModel* GetBookmarkModel() = 0;
   virtual favicon::FaviconService* GetFaviconService() = 0;
   virtual history::HistoryService* GetHistoryService() = 0;
+  virtual sync_preferences::PrefServiceSyncable* GetPrefServiceSyncable() = 0;
   virtual sync_sessions::SessionSyncService* GetSessionSyncService() = 0;
   virtual send_tab_to_self::SendTabToSelfSyncService*
   GetSendTabToSelfSyncService() = 0;

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -41,6 +41,7 @@
 #include "components/sync/model_impl/proxy_model_type_controller_delegate.h"
 #include "components/sync_bookmarks/bookmark_sync_service.h"
 #include "components/sync_device_info/device_info_sync_service.h"
+#include "components/sync_preferences/pref_service_syncable.h"
 #include "components/sync_sessions/proxy_tabs_data_type_controller.h"
 #include "components/sync_sessions/session_model_type_controller.h"
 #include "components/sync_sessions/session_sync_service.h"
@@ -104,6 +105,14 @@ AutofillWalletOfferDelegateFromDataService(
       ->GetControllerDelegate();
 }
 
+// Helper function that deals will null (e.g. tests, iOS webview).
+base::WeakPtr<syncer::SyncableService> SyncableServiceForPrefs(
+    sync_preferences::PrefServiceSyncable* prefs_service,
+    syncer::ModelType type) {
+  return prefs_service ? prefs_service->GetSyncableService(type)->AsWeakPtr()
+                       : nullptr;
+}
+
 }  // namespace
 
 ProfileSyncComponentsFactoryImpl::ProfileSyncComponentsFactoryImpl(
@@ -254,9 +263,10 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
     // Delete directive sync is enabled by default.
     if (!disabled_types.Has(syncer::HISTORY_DELETE_DIRECTIVES)) {
       controllers.push_back(
-          std::make_unique<HistoryDeleteDirectivesModelTypeController>(
+          std::make_unique<history::HistoryDeleteDirectivesModelTypeController>(
               dump_stack, sync_service,
-              sync_client_->GetModelTypeStoreService(), sync_client_));
+              sync_client_->GetModelTypeStoreService(),
+              sync_client_->GetHistoryService()));
     }
 
     // Session sync is enabled by default.  This is disabled if history is
@@ -301,7 +311,8 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
         std::make_unique<SyncableServiceBasedModelTypeController>(
             syncer::PREFERENCES,
             sync_client_->GetModelTypeStoreService()->GetStoreFactory(),
-            sync_client_->GetSyncableServiceForType(syncer::PREFERENCES),
+            SyncableServiceForPrefs(sync_client_->GetPrefServiceSyncable(),
+                                    syncer::PREFERENCES),
             dump_stack));
   }
 
@@ -310,7 +321,7 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
         std::make_unique<SyncableServiceBasedModelTypeController>(
             syncer::PRIORITY_PREFERENCES,
             sync_client_->GetModelTypeStoreService()->GetStoreFactory(),
-            sync_client_->GetSyncableServiceForType(
+            SyncableServiceForPrefs(sync_client_->GetPrefServiceSyncable(),
                                     syncer::PRIORITY_PREFERENCES),
             dump_stack));
   }

components/history/core/browser/sync/history_delete_directives_model_type_controller.cc

@@ -7,24 +7,36 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "components/sync/driver/sync_client.h"
+#include "base/memory/weak_ptr.h"
+#include "components/history/core/browser/history_service.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
 #include "components/sync/model/model_type_store_service.h"
 
-namespace browser_sync {
+namespace history {
+
+namespace {
+
+base::WeakPtr<syncer::SyncableService> GetSyncableServiceFromHistoryService(
+    HistoryService* history_service) {
+  if (history_service) {
+    return history_service->GetDeleteDirectivesSyncableService();
+  }
+  return nullptr;
+}
+
+}  // namespace
 
 HistoryDeleteDirectivesModelTypeController::
     HistoryDeleteDirectivesModelTypeController(
         const base::RepeatingClosure& dump_stack,
         syncer::SyncService* sync_service,
         syncer::ModelTypeStoreService* model_type_store_service,
-        syncer::SyncClient* sync_client)
+        HistoryService* history_service)
     : SyncableServiceBasedModelTypeController(
           syncer::HISTORY_DELETE_DIRECTIVES,
           model_type_store_service->GetStoreFactory(),
-          sync_client->GetSyncableServiceForType(
-              syncer::HISTORY_DELETE_DIRECTIVES),
+          GetSyncableServiceFromHistoryService(history_service),
           dump_stack),
       sync_service_(sync_service) {}
 
@@ -68,4 +80,4 @@ void HistoryDeleteDirectivesModelTypeController::OnStateChanged(
   sync_service_->DataTypePreconditionChanged(type());
 }
 
-}  // namespace browser_sync
+}  // namespace history

components/history/core/browser/sync/history_delete_directives_model_type_controller.h

@@ -12,11 +12,12 @@
 
 namespace syncer {
 class ModelTypeStoreService;
-class SyncClient;
 class SyncService;
 }  // namespace syncer
 
-namespace browser_sync {
+namespace history {
+
+class HistoryService;
 
 // A controller for delete directives, which cannot sync when full encryption
 // is enabled.
@@ -24,13 +25,13 @@ class HistoryDeleteDirectivesModelTypeController
     : public syncer::SyncableServiceBasedModelTypeController,
       public syncer::SyncServiceObserver {
  public:
-  // |sync_service| and |sync_client| must not be null and must outlive this
+  // |sync_service| and |history_service| must not be null and must outlive this
   // object.
   HistoryDeleteDirectivesModelTypeController(
       const base::RepeatingClosure& dump_stack,
       syncer::SyncService* sync_service,
       syncer::ModelTypeStoreService* model_type_store_service,
-      syncer::SyncClient* sync_client);
+      HistoryService* history_service);
   ~HistoryDeleteDirectivesModelTypeController() override;
 
   // DataTypeController overrides.
@@ -49,6 +50,6 @@ class HistoryDeleteDirectivesModelTypeController
   DISALLOW_COPY_AND_ASSIGN(HistoryDeleteDirectivesModelTypeController);
 };
 
-}  // namespace browser_sync
+}  // namespace history
 
 #endif  // COMPONENTS_HISTORY_CORE_BROWSER_SYNC_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_

components/sync/driver/sync_client.h

@@ -7,9 +7,7 @@
 
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
-#include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "components/sync/base/extensions_activity.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/data_type_controller.h"
@@ -28,7 +26,6 @@ class IdentityManager;
 namespace syncer {
 
 class SyncApiComponentFactory;
-class SyncableService;
 class SyncInvalidationsService;
 class SyncService;
 class SyncTypePreferenceProvider;
@@ -43,6 +40,8 @@ class TrustedVaultClient;
 class SyncClient {
  public:
   SyncClient() = default;
+  SyncClient(const SyncClient&) = delete;
+  SyncClient& operator=(const SyncClient&) = delete;
   virtual ~SyncClient() = default;
 
   // Returns the current profile's preference service.
@@ -65,12 +64,6 @@ class SyncClient {
   virtual TrustedVaultClient* GetTrustedVaultClient() = 0;
   virtual scoped_refptr<ExtensionsActivity> GetExtensionsActivity() = 0;
 
-  // Returns a weak pointer to the syncable service specified by |type|.
-  // Weak pointer may be unset if service is already destroyed.
-  // Note: Should only be dereferenced from the model type thread.
-  virtual base::WeakPtr<SyncableService> GetSyncableServiceForType(
-      ModelType type) = 0;
-
   // Creates and returns a new ModelSafeWorker for the group, or null if one
   // cannot be created.
   // TODO(maxbogue): Move this inside SyncApiComponentFactory.
@@ -82,9 +75,6 @@ class SyncClient {
 
   // Returns the preference provider, or null if none exists.
   virtual SyncTypePreferenceProvider* GetPreferenceProvider() = 0;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SyncClient);
 };
 
 }  // namespace syncer

components/sync/driver/sync_client_mock.h

@@ -29,8 +29,6 @@ class SyncClientMock : public SyncClient {
                syncer::SyncInvalidationsService*());
   MOCK_METHOD0(GetTrustedVaultClient, TrustedVaultClient*());
   MOCK_METHOD0(GetExtensionsActivity, scoped_refptr<ExtensionsActivity>());
-  MOCK_METHOD1(GetSyncableServiceForType,
-               base::WeakPtr<SyncableService>(ModelType type));
   MOCK_METHOD1(CreateModelWorkerForGroup,
                scoped_refptr<ModelSafeWorker>(ModelSafeGroup group));
   MOCK_METHOD0(GetSyncApiComponentFactory, SyncApiComponentFactory*());

components/sync_preferences/pref_service_syncable.h

@@ -82,8 +82,6 @@ class PrefServiceSyncable : public PrefService {
   void AddObserver(PrefServiceSyncableObserver* observer);
   void RemoveObserver(PrefServiceSyncableObserver* observer);
 
-  // TODO(zea): Have PrefServiceSyncable implement
-  // syncer::SyncableService directly.
   syncer::SyncableService* GetSyncableService(const syncer::ModelType& type);
 
   // Do not call this after having derived an incognito or per tab pref service.

ios/chrome/browser/sync/ios_chrome_sync_client.h

@@ -43,6 +43,7 @@ class IOSChromeSyncClient : public browser_sync::BrowserSyncClient {
   bookmarks::BookmarkModel* GetBookmarkModel() override;
   favicon::FaviconService* GetFaviconService() override;
   history::HistoryService* GetHistoryService() override;
+  sync_preferences::PrefServiceSyncable* GetPrefServiceSyncable() override;
   sync_sessions::SessionSyncService* GetSessionSyncService() override;
   base::Closure GetPasswordStateChangedCallback() override;
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
@@ -52,8 +53,6 @@ class IOSChromeSyncClient : public browser_sync::BrowserSyncClient {
   syncer::TrustedVaultClient* GetTrustedVaultClient() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;
-  base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
-      syncer::ModelType type) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate>
   GetControllerDelegateForModelType(syncer::ModelType type) override;
   scoped_refptr<syncer::ModelSafeWorker> CreateModelWorkerForGroup(

ios/chrome/browser/sync/ios_chrome_sync_client.mm

@@ -35,7 +35,6 @@
 #include "components/sync/driver/sync_api_component_factory.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/engine/passive_model_worker.h"
-#include "components/sync_preferences/pref_service_syncable.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_user_events/user_event_service.h"
 #include "ios/chrome/browser/bookmarks/bookmark_model_factory.h"
@@ -162,6 +161,12 @@ history::HistoryService* IOSChromeSyncClient::GetHistoryService() {
       browser_state_, ServiceAccessType::EXPLICIT_ACCESS);
 }
 
+sync_preferences::PrefServiceSyncable*
+IOSChromeSyncClient::GetPrefServiceSyncable() {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  return browser_state_->GetSyncablePrefs();
+}
+
 sync_sessions::SessionSyncService*
 IOSChromeSyncClient::GetSessionSyncService() {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
@@ -210,30 +215,6 @@ IOSChromeSyncClient::GetExtensionsActivity() {
   return nullptr;
 }
 
-base::WeakPtr<syncer::SyncableService>
-IOSChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
-  switch (type) {
-    case syncer::PREFERENCES:
-      return browser_state_->GetSyncablePrefs()
-          ->GetSyncableService(syncer::PREFERENCES)
-          ->AsWeakPtr();
-    case syncer::PRIORITY_PREFERENCES:
-      return browser_state_->GetSyncablePrefs()
-          ->GetSyncableService(syncer::PRIORITY_PREFERENCES)
-          ->AsWeakPtr();
-    case syncer::HISTORY_DELETE_DIRECTIVES: {
-      history::HistoryService* history =
-          ios::HistoryServiceFactory::GetForBrowserState(
-              browser_state_, ServiceAccessType::EXPLICIT_ACCESS);
-      return history ? history->GetDeleteDirectivesSyncableService()
-                     : base::WeakPtr<syncer::SyncableService>();
-    }
-    default:
-      NOTREACHED();
-      return base::WeakPtr<syncer::SyncableService>();
-  }
-}
-
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 IOSChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
   switch (type) {

ios/web_view/internal/sync/web_view_sync_client.h

@@ -47,6 +47,7 @@ class WebViewSyncClient : public browser_sync::BrowserSyncClient {
   history::HistoryService* GetHistoryService() override;
   send_tab_to_self::SendTabToSelfSyncService* GetSendTabToSelfSyncService()
       override;
+  sync_preferences::PrefServiceSyncable* GetPrefServiceSyncable() override;
   sync_sessions::SessionSyncService* GetSessionSyncService() override;
   base::RepeatingClosure GetPasswordStateChangedCallback() override;
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
@@ -56,8 +57,6 @@ class WebViewSyncClient : public browser_sync::BrowserSyncClient {
   syncer::TrustedVaultClient* GetTrustedVaultClient() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;
-  base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(
-      syncer::ModelType type) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate>
   GetControllerDelegateForModelType(syncer::ModelType type) override;
   scoped_refptr<syncer::ModelSafeWorker> CreateModelWorkerForGroup(

ios/web_view/internal/sync/web_view_sync_client.mm

@@ -147,6 +147,11 @@ history::HistoryService* WebViewSyncClient::GetHistoryService() {
   return nullptr;
 }
 
+sync_preferences::PrefServiceSyncable*
+WebViewSyncClient::GetPrefServiceSyncable() {
+  return nullptr;
+}
+
 sync_sessions::SessionSyncService* WebViewSyncClient::GetSessionSyncService() {
   return nullptr;
 }
@@ -189,12 +194,6 @@ WebViewSyncClient::GetExtensionsActivity() {
   return nullptr;
 }
 
-base::WeakPtr<syncer::SyncableService>
-WebViewSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
-  NOTREACHED();
-  return base::WeakPtr<syncer::SyncableService>();
-}
-
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 WebViewSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
   NOTREACHED();