[Device info sync] Construct the controller in ProfileSyncService

This CL moves the construction of the sync controller for device info
directly into ProfileSyncService that owns the DeviceInfoSyncBridge.
This (a) heavily simplifies how the controller gets access to its
delegate (removing the accessor from SyncClients) and (b) allows to use
the forwarding delegate instead of the proxy delegate.

This CL is a preparation for using the simpler forwarding delegate for
all sync types that run on the UI sequence.

This CL also resolves a TODO by making it clear that DEVICE_INFO cannot
get disabled via the command-line switch.

Bug: 867801
Change-Id: Iea84d818613476f40d89eebd73266aa24d028fb6
Reviewed-on: https://chromium-review.googlesource.com/c/1348092
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610448}

chrome/browser/sync/chrome_sync_client.cc

@@ -151,7 +151,13 @@ syncer::ModelTypeSet GetDisabledTypesFromCommandLine() {
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kDisableSyncTypes);
 
-  return syncer::ModelTypeSetFromString(disabled_types_str);
+  syncer::ModelTypeSet disabled_types =
+      syncer::ModelTypeSetFromString(disabled_types_str);
+  if (disabled_types.Has(syncer::DEVICE_INFO)) {
+    DLOG(WARNING) << "DEVICE_INFO cannot be disabled via a command-line switch";
+    disabled_types.Remove(syncer::DEVICE_INFO);
+  }
+  return disabled_types;
 }
 
 }  // namespace
@@ -597,9 +603,6 @@ ChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 ChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
   switch (type) {
-    case syncer::DEVICE_INFO:
-      return ProfileSyncServiceFactory::GetForProfile(profile_)
-          ->GetDeviceInfoSyncControllerDelegate();
     case syncer::READING_LIST:
       // Reading List is only supported on iOS at the moment.
       NOTREACHED();
@@ -628,6 +631,7 @@ ChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
     case syncer::AUTOFILL_WALLET_DATA:
     case syncer::AUTOFILL_WALLET_METADATA:
     case syncer::BOOKMARKS:
+    case syncer::DEVICE_INFO:
     case syncer::SESSIONS:
     case syncer::TYPED_URLS:
       NOTREACHED();

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -141,11 +141,6 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
   const base::RepeatingClosure dump_stack =
       base::BindRepeating(&syncer::ReportUnrecoverableError, channel_);
 
-  // TODO(stanisc): can DEVICE_INFO be one of disabled datatypes?
-  // Use an error callback that always uploads a stacktrace if it can to help
-  // get USS as stable as possible.
-  controllers.push_back(
-      CreateModelTypeControllerForModelRunningOnUIThread(syncer::DEVICE_INFO));
   // These features are enabled only if there's a DB thread to post tasks to.
   if (db_thread_) {
     // Autocomplete sync is enabled by default.  Register unless explicitly

components/browser_sync/profile_sync_service.cc

@@ -36,6 +36,7 @@
 #include "components/sync/driver/clear_server_data_events.h"
 #include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/directory_data_type_controller.h"
+#include "components/sync/driver/model_type_controller.h"
 #include "components/sync/driver/sync_api_component_factory.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_type_preference_provider.h"
@@ -52,6 +53,7 @@
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync/model_impl/forwarding_model_type_controller_delegate.h"
 #include "components/sync/syncable/base_transaction.h"
 #include "components/sync/syncable/directory.h"
 #include "components/sync_sessions/session_sync_service.h"
@@ -251,15 +253,22 @@ void ProfileSyncService::Initialize() {
       sync_service_url_, local_device_->GetSyncUserAgent(), url_loader_factory_,
       syncer::SyncStoppedReporter::ResultCallback());
 
+  data_type_controllers_ =
+      BuildDataTypeControllerMap(sync_client_->CreateDataTypeControllers());
+
   device_info_sync_bridge_ = std::make_unique<syncer::DeviceInfoSyncBridge>(
       local_device_.get(), model_type_store_factory,
       std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
           syncer::DEVICE_INFO,
           /*dump_stack=*/base::BindRepeating(&syncer::ReportUnrecoverableError,
                                              channel_)));
-
-  data_type_controllers_ =
-      BuildDataTypeControllerMap(sync_client_->CreateDataTypeControllers());
+  data_type_controllers_[syncer::DEVICE_INFO] =
+      std::make_unique<syncer::ModelTypeController>(
+          syncer::DEVICE_INFO,
+          std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+              device_info_sync_bridge_->change_processor()
+                  ->GetControllerDelegate()
+                  .get()));
 
   if (gaia_cookie_manager_service_)
     gaia_cookie_manager_service_->AddObserver(this);
@@ -2052,12 +2061,6 @@ std::string ProfileSyncService::GetAccessTokenForTest() const {
   return auth_manager_->access_token();
 }
 
-base::WeakPtr<syncer::ModelTypeControllerDelegate>
-ProfileSyncService::GetDeviceInfoSyncControllerDelegate() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return device_info_sync_bridge_->change_processor()->GetControllerDelegate();
-}
-
 syncer::SyncTokenStatus ProfileSyncService::GetSyncTokenStatus() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return auth_manager_->GetSyncTokenStatus();

components/browser_sync/profile_sync_service.h

@@ -60,7 +60,6 @@ namespace syncer {
 class BackendMigrator;
 class DeviceInfoSyncBridge;
 class DeviceInfoTracker;
-class ModelTypeControllerDelegate;
 class NetworkResources;
 class SyncTypePreferenceProvider;
 class TypeDebugInfoObserver;
@@ -291,10 +290,6 @@ class ProfileSyncService : public syncer::SyncService,
 
   syncer::LocalDeviceInfoProvider* GetLocalDeviceInfoProviderForTest();
 
-  // Returns the ModelTypeControllerDelegate for syncer::DEVICE_INFO.
-  base::WeakPtr<syncer::ModelTypeControllerDelegate>
-  GetDeviceInfoSyncControllerDelegate();
-
   // Returns synced devices tracker.
   // Virtual for testing.
   virtual syncer::DeviceInfoTracker* GetDeviceInfoTracker() const;

components/browser_sync/profile_sync_service_autofill_unittest.cc

@@ -355,8 +355,13 @@ class ProfileSyncServiceAutofillTest
   void OnDataTypeConfigureComplete(
       const std::vector<syncer::DataTypeConfigurationStats>&
           configuration_stats) override {
-    ASSERT_EQ(1u, configuration_stats.size());
-    association_stats_ = configuration_stats[0].association_stats;
+    for (const syncer::DataTypeConfigurationStats& stat : configuration_stats) {
+      if (stat.model_type == syncer::AUTOFILL_PROFILE) {
+        association_stats_ = stat.association_stats;
+        return;
+      }
+    }
+    ASSERT_TRUE(false) << "Autofill profile type did not get configured!";
   }
 
  protected:

ios/chrome/browser/sync/ios_chrome_sync_client.mm

@@ -83,7 +83,13 @@ syncer::ModelTypeSet GetDisabledTypesFromCommandLine() {
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kDisableSyncTypes);
 
-  return syncer::ModelTypeSetFromString(disabled_types_str);
+  syncer::ModelTypeSet disabled_types =
+      syncer::ModelTypeSetFromString(disabled_types_str);
+  if (disabled_types.Has(syncer::DEVICE_INFO)) {
+    DLOG(WARNING) << "DEVICE_INFO cannot be disabled via a command-line switch";
+    disabled_types.Remove(syncer::DEVICE_INFO);
+  }
+  return disabled_types;
 }
 
 }  // namespace
@@ -276,9 +282,6 @@ IOSChromeSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 IOSChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
   switch (type) {
-    case syncer::DEVICE_INFO:
-      return ProfileSyncServiceFactory::GetForBrowserState(browser_state_)
-          ->GetDeviceInfoSyncControllerDelegate();
     case syncer::READING_LIST: {
       ReadingListModel* reading_list_model =
           ReadingListModelFactory::GetForBrowserState(browser_state_);
@@ -302,6 +305,7 @@ IOSChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
     case syncer::AUTOFILL_WALLET_DATA:
     case syncer::AUTOFILL_WALLET_METADATA:
     case syncer::BOOKMARKS:
+    case syncer::DEVICE_INFO:
     case syncer::SESSIONS:
     case syncer::TYPED_URLS:
       NOTREACHED();

ios/web_view/internal/sync/web_view_sync_client.mm

@@ -196,17 +196,9 @@ WebViewSyncClient::GetSyncableServiceForType(syncer::ModelType type) {
 
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 WebViewSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
-  switch (type) {
-    case syncer::DEVICE_INFO:
-      // TODO(crbug.com/872420): Distinguish ios/web_view from ios/chrome.
-      return WebViewProfileSyncServiceFactory::GetForBrowserState(
-                 browser_state_)
-          ->GetDeviceInfoSyncControllerDelegate();
-    default:
-      NOTREACHED();
-      // TODO(crbug.com/873790): Figure out if USER_CONSENTS need to be enabled.
-      return base::WeakPtr<syncer::ModelTypeControllerDelegate>();
-  }
+  NOTREACHED();
+  // TODO(crbug.com/873790): Figure out if USER_CONSENTS need to be enabled.
+  return base::WeakPtr<syncer::ModelTypeControllerDelegate>();
 }
 
 scoped_refptr<syncer::ModelSafeWorker>