[sync] Refactor DataTypeController::Connect()'s signature

This addresses a TODO to avoid injecting ModelTypeConfigurer into
controllers, and instead simply return the ActivationResponse to
the calling site (which can interact with ModelTypeConfigurer).

The only exceptional case for this is PROXY_TABS, which doesn't
Connect(). The chosen approach in this patch is that PROXY_TABS
bypasses the MODEL_LOADED state and goes directly into RUNNING state,
which is somewhat subtle and subject to change in future patches.

Change-Id: I9ab3ebeb0743d9ca99b04668b9b0b80c1274b52f
Fixed: 967677
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3069268
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#908811}

components/sync/driver/data_type_controller.h

@@ -21,35 +21,28 @@
 namespace syncer {
 
 struct ConfigureContext;
+struct DataTypeActivationResponse;
 struct TypeEntitiesCount;
-class ModelTypeConfigurer;
 class SyncError;
 
 // DataTypeControllers are responsible for managing the state of a single data
 // type. They are not thread safe and should only be used on the UI thread.
 class DataTypeController : public base::SupportsWeakPtr<DataTypeController> {
  public:
+  // TODO(crbug.com/967677): Should MODEL_LOADED be renamed to
+  // MODEL_READY_TO_CONNECT?
   enum State {
     NOT_RUNNING,     // The controller has never been started or has previously
                      // been stopped.  Must be in this state to start.
     MODEL_STARTING,  // The model is loading.
-    MODEL_LOADED,    // The model has finished loading and can start running.
+    MODEL_LOADED,    // The model has finished loading and is ready to connect.
     RUNNING,         // The controller is running and the data type is
                      // in sync with the cloud.
     STOPPING,        // The controller is in the process of stopping
                      // and is waiting for dependent services to stop.
     FAILED           // The controller was started but encountered an error.
   };
 
-  // Returned from Connect.
-  enum ConnectResult {
-    // Indicates that the initial download for this type is already complete, or
-    // wasn't needed in the first place (e.g. for proxy types).
-    TYPE_ALREADY_DOWNLOADED,
-    // Indicates that the initial download for this type still needs to be done.
-    TYPE_NOT_YET_DOWNLOADED,
-  };
-
   // Note: This seems like it should be a OnceCallback, but it can actually be
   // called multiple times in the case of errors.
   using ModelLoadCallback =
@@ -76,14 +69,11 @@ class DataTypeController : public base::SupportsWeakPtr<DataTypeController> {
   virtual void LoadModels(const ConfigureContext& configure_context,
                           const ModelLoadCallback& model_load_callback) = 0;
 
-  // Called by DataTypeManager once the local model has loaded, but before
-  // downloading initial data (if necessary). Returns whether the initial
-  // download for this type is already complete.
-  virtual ConnectResult Connect(ModelTypeConfigurer* configurer) = 0;
-
-  // Called by DataTypeManager to disconnect the controlled data type.
-  // See comments for ModelAssociationManager::OnSingleDataTypeWillStop.
-  virtual void Disconnect(ModelTypeConfigurer* configurer) = 0;
+  // Called by DataTypeManager once the local model has loaded (MODEL_LOADED),
+  // in order to enable the sync engine's propagation of sync changes between
+  // the server and the local processor. Upon return, the controller assumes
+  // that the caller will take care of actually instrumenting the sync engine.
+  virtual std::unique_ptr<DataTypeActivationResponse> Connect() = 0;
 
   // Stops the data type. If LoadModels() has not completed it will enter
   // STOPPING state first and eventually STOPPED. Once stopped, |callback| will

components/sync/driver/data_type_manager_impl.cc

@@ -4,6 +4,7 @@
 
 #include "components/sync/driver/data_type_manager_impl.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
@@ -19,6 +20,7 @@
 #include "components/sync/driver/data_type_encryption_handler.h"
 #include "components/sync/driver/data_type_manager_observer.h"
 #include "components/sync/driver/data_type_status_table.h"
+#include "components/sync/engine/data_type_activation_response.h"
 #include "components/sync/engine/data_type_debug_info_listener.h"
 
 namespace syncer {
@@ -191,29 +193,33 @@ void DataTypeManagerImpl::ConfigureImpl(ModelTypeSet desired_types,
 void DataTypeManagerImpl::ConnectDataTypes() {
   for (ModelType type : last_enabled_types_) {
     const auto& dtc_iter = controllers_->find(type);
-    if (dtc_iter == controllers_->end())
+    if (dtc_iter == controllers_->end()) {
       continue;
+    }
     DataTypeController* dtc = dtc_iter->second.get();
-    if (dtc->state() == DataTypeController::MODEL_LOADED) {
-      // Only call Connect() for types that completed LoadModels()
-      // successfully. Such types shouldn't be in an error state at the same
-      // time.
-      DCHECK(!data_type_status_table_.GetFailedTypes().Has(dtc->type()));
-      switch (dtc->Connect(configurer_)) {
-        case DataTypeController::TYPE_ALREADY_DOWNLOADED:
-          // Proxy types (as opposed to protocol types) don't actually have any
-          // data, so keep proxy types out of |downloaded_types_|.
-          if (!IsProxyType(type))
-            downloaded_types_.Put(type);
-          break;
-        case DataTypeController::TYPE_NOT_YET_DOWNLOADED:
-          downloaded_types_.Remove(type);
-          break;
-      }
-      if (force_redownload_types_.Has(type)) {
-        downloaded_types_.Remove(type);
-      }
+    if (dtc->state() != DataTypeController::MODEL_LOADED) {
+      continue;
+    }
+    // Only call Connect() for types that completed LoadModels()
+    // successfully. Such types shouldn't be in an error state at the same
+    // time.
+    DCHECK(!data_type_status_table_.GetFailedTypes().Has(dtc->type()));
+
+    std::unique_ptr<DataTypeActivationResponse> activation_response =
+        dtc->Connect();
+    DCHECK(activation_response);
+    DCHECK_EQ(dtc->state(), DataTypeController::RUNNING);
+
+    if (activation_response->model_type_state.initial_sync_done()) {
+      downloaded_types_.Put(type);
+    } else {
+      downloaded_types_.Remove(type);
     }
+    if (force_redownload_types_.Has(type)) {
+      downloaded_types_.Remove(type);
+    }
+
+    configurer_->ConnectDataType(type, std::move(activation_response));
   }
 }
 
@@ -339,6 +345,14 @@ void DataTypeManagerImpl::OnAllDataTypesReadyForConfigure() {
   // could have failed loading and should be excluded from configuration. I need
   // to adjust |configuration_types_queue_| for such types.
   ConnectDataTypes();
+
+  // Propagate the state of PROXY_TABS to the sync engine.
+  const auto& dtc_iter = controllers_->find(PROXY_TABS);
+  if (dtc_iter != controllers_->end()) {
+    configurer_->SetProxyTabsDatatypeEnabled(dtc_iter->second->state() ==
+                                             DataTypeController::RUNNING);
+  }
+
   StartNextConfiguration(/*higher_priority_types_before=*/ModelTypeSet());
 }
 
@@ -618,10 +632,8 @@ void DataTypeManagerImpl::RecordConfigurationStats(
 
 void DataTypeManagerImpl::OnSingleDataTypeWillStop(ModelType type,
                                                    const SyncError& error) {
-  auto c_it = controllers_->find(type);
-  DCHECK(c_it != controllers_->end());
-  // Delegate deactivation to the controller.
-  c_it->second->Disconnect(configurer_);
+  // No-op if the type is not connected.
+  configurer_->DisconnectDataType(type);
 
   if (error.IsSet()) {
     data_type_status_table_.UpdateFailedDataType(type, error);

components/sync/driver/fake_data_type_controller.cc

@@ -6,6 +6,8 @@
 
 #include <memory>
 
+#include "components/sync/engine/data_type_activation_response.h"
+
 namespace syncer {
 
 FakeDataTypeController::FakeDataTypeController(ModelType type)
@@ -39,10 +41,9 @@ FakeDataTypeController::GetPreconditionState() const {
   return precondition_state_;
 }
 
-DataTypeController::ConnectResult FakeDataTypeController::Connect(
-    ModelTypeConfigurer* configurer) {
+std::unique_ptr<DataTypeActivationResponse> FakeDataTypeController::Connect() {
   ++activate_call_count_;
-  return ModelTypeController::Connect(configurer);
+  return ModelTypeController::Connect();
 }
 
 }  // namespace syncer

components/sync/driver/fake_data_type_controller.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_SYNC_DRIVER_FAKE_DATA_TYPE_CONTROLLER_H__
 #define COMPONENTS_SYNC_DRIVER_FAKE_DATA_TYPE_CONTROLLER_H__
 
+#include <memory>
+
 #include "components/sync/base/sync_mode.h"
 #include "components/sync/driver/model_type_controller.h"
 #include "components/sync/test/model/fake_model_type_controller_delegate.h"
@@ -29,7 +31,7 @@ class FakeDataTypeController : public ModelTypeController {
 
   // ModelTypeController overrides.
   PreconditionState GetPreconditionState() const override;
-  ConnectResult Connect(ModelTypeConfigurer* configurer) override;
+  std::unique_ptr<DataTypeActivationResponse> Connect() override;
 
  private:
   PreconditionState precondition_state_ = PreconditionState::kPreconditionsMet;

components/sync/driver/glue/sync_engine_backend.cc

@@ -519,7 +519,7 @@ void SyncEngineBackend::LoadAndConnectNigoriController() {
   DCHECK_EQ(nigori_controller_->state(), DataTypeController::MODEL_LOADED);
   // TODO(crbug.com/922900): Do we need to call RegisterDataType() for Nigori?
   sync_manager_->GetModelTypeConnector()->ConnectDataType(
-      NIGORI, nigori_controller_->ActivateManuallyForNigori());
+      NIGORI, nigori_controller_->Connect());
 }
 
 }  // namespace syncer

components/sync/driver/glue/sync_engine_impl.cc

@@ -352,7 +352,6 @@ void SyncEngineImpl::ConnectDataType(
 }
 
 void SyncEngineImpl::DisconnectDataType(ModelType type) {
-  DCHECK(!IsProxyType(type));
   model_type_connector_->DisconnectDataType(type);
 }
 

components/sync/driver/model_type_controller.cc

@@ -14,7 +14,6 @@
 #include "components/sync/base/data_type_histogram.h"
 #include "components/sync/driver/configure_context.h"
 #include "components/sync/engine/data_type_activation_response.h"
-#include "components/sync/engine/model_type_configurer.h"
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync/model/data_type_error_handler_impl.h"
 #include "components/sync/model/type_entities_count.h"
@@ -77,16 +76,6 @@ void ModelTypeController::InitModelTypeController(
   }
 }
 
-std::unique_ptr<DataTypeActivationResponse>
-ModelTypeController::ActivateManuallyForNigori() {
-  // To avoid abuse of this temporary API, we restrict it to NIGORI.
-  DCHECK_EQ(NIGORI, type());
-  DCHECK_EQ(MODEL_LOADED, state_);
-  DCHECK(activation_response_);
-  state_ = RUNNING;
-  return std::move(activation_response_);
-}
-
 void ModelTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
@@ -122,32 +111,15 @@ void ModelTypeController::LoadModels(
                               base::AsWeakPtr(this)));
 }
 
-DataTypeController::ConnectResult ModelTypeController::Connect(
-    ModelTypeConfigurer* configurer) {
+std::unique_ptr<DataTypeActivationResponse> ModelTypeController::Connect() {
   DCHECK(CalledOnValidThread());
-  DCHECK(configurer);
   DCHECK(activation_response_);
   DCHECK_EQ(MODEL_LOADED, state_);
 
-  bool initial_sync_done =
-      activation_response_->model_type_state.initial_sync_done();
-  // Pass activation context to ModelTypeRegistry, where ModelTypeWorker gets
-  // created and connected with the delegate (processor).
-  configurer->ConnectDataType(type(), std::move(activation_response_));
-
   state_ = RUNNING;
   DVLOG(1) << "Sync running for " << ModelTypeToString(type());
 
-  return initial_sync_done ? TYPE_ALREADY_DOWNLOADED : TYPE_NOT_YET_DOWNLOADED;
-}
-
-void ModelTypeController::Disconnect(ModelTypeConfigurer* configurer) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(configurer);
-  if (state_ == RUNNING) {
-    configurer->DisconnectDataType(type());
-    state_ = MODEL_LOADED;
-  }
+  return std::move(activation_response_);
 }
 
 void ModelTypeController::Stop(ShutdownReason reason, StopCallback callback) {

components/sync/driver/model_type_controller.h

@@ -38,17 +38,10 @@ class ModelTypeController : public DataTypeController {
       std::unique_ptr<ModelTypeControllerDelegate> delegate_for_transport_mode);
   ~ModelTypeController() override;
 
-  // Steals the activation response, only used for Nigori.
-  // TODO(crbug.com/967677): Once all datatypes are in USS, we should redesign
-  // or remove Connect, and expose the activation response via
-  // LoadModels(), which is more natural in USS.
-  std::unique_ptr<DataTypeActivationResponse> ActivateManuallyForNigori();
-
   // DataTypeController implementation.
   void LoadModels(const ConfigureContext& configure_context,
                   const ModelLoadCallback& model_load_callback) override;
-  ConnectResult Connect(ModelTypeConfigurer* configurer) override;
-  void Disconnect(ModelTypeConfigurer* configurer) override;
+  std::unique_ptr<DataTypeActivationResponse> Connect() override;
   void Stop(ShutdownReason reason, StopCallback callback) override;
   State state() const override;
   bool ShouldRunInTransportOnlyMode() const override;