[Sync] Refactor sync configuration logic.

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 66f6307d06b7df49782c332c6ab7db6b6da5aad0..470989a02ae88e775970591310b24406be4abf5d 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -131,10 +131,6 @@ class SyncBackendHost::Core
   // Called to clear server data.
   void DoRequestClearServerData();
 
-  // Called to cleanup disabled types.
-  void DoRequestCleanupDisabledTypes(
-      const browser_sync::ModelSafeRoutingInfo& routing_info);
-
   // Called to set the passphrase for encryption.
   void DoSetEncryptionPassphrase(const std::string& passphrase,
                                  bool is_explicit);
@@ -164,14 +160,18 @@ class SyncBackendHost::Core
   void DoStopSyncManagerForShutdown(const base::Closure& closure);
   void DoShutdown(bool stopping_sync);
 
-  virtual void DoRequestConfig(
-      const browser_sync::ModelSafeRoutingInfo& routing_info,
+  // Configuration methods that must execute on sync loop.
+  void DoConfigureSyncer(
+      sync_api::ConfigureReason reason,
       syncable::ModelTypeSet types_to_config,
-      sync_api::ConfigureReason reason);
-
-  // Start the configuration mode.  |callback| is called on the sync
-  // thread.
-  virtual void DoStartConfiguration(const base::Closure& callback);
+      const browser_sync::ModelSafeRoutingInfo routing_info,
+      const base::Callback<void(syncable::ModelTypeSet)>& ready_task,
+      const base::Closure& retry_callback);
+  void DoFinishConfigureDataTypes(
+      syncable::ModelTypeSet types_to_config,
+      const base::Callback<void(syncable::ModelTypeSet)>& ready_task);
+  void DoRetryConfiguration(
+      const base::Closure& retry_callback);
 
   // Set the base request context to use when making HTTP calls.
   // This method will add a reference to the context to persist it
@@ -184,9 +184,6 @@ class SyncBackendHost::Core
   // sync databases), as well as shutdown when you're no longer syncing.
   void DeleteSyncDataFolder();
 
-  // A callback from the SyncerThread when it is safe to continue config.
-  void FinishConfigureDataTypes();
-
  private:
   friend class base::RefCountedThreadSafe<SyncBackendHost::Core>;
   friend class SyncBackendHostForProfileSyncTest;
@@ -556,60 +553,79 @@ void SyncBackendHost::Shutdown(bool sync_disabled) {
   core_ = NULL;  // Releases reference to core_.
 }
 
+// TODO(sync): remove |disabled_types| completely, as it's just the set of
+// registered types - the set of desired types.

[tim (not reviewing), 2012/06/14 00:26:53]

I originally wrote it that way, but I think some folks (Fred, I think?)
preferred to have this API be explicit about added and removed types. See
changes to sync_backend_host.cc in http://codereview.chromium.org/7511004. 

Should we have a referendum and write the outcome it in a comment? :P

[Nicolas Zea, 2012/06/14 22:35:52]

Well, previously we had to rely on having the set of datatype controllers around
to determine this, and i don't think we could rely on the registrar to get it at
that point? I think with all the changes recently we might be able to in a
clean/reliable way, but I'd need to dig a bit more to be certain.

 void SyncBackendHost::ConfigureDataTypes(
     sync_api::ConfigureReason reason,
-    syncable::ModelTypeSet types_to_add,
-    syncable::ModelTypeSet types_to_remove,
+    const syncable::ModelTypeSet& desired_types,
+    const syncable::ModelTypeSet& disabled_types,
     NigoriState nigori_state,
-    base::Callback<void(syncable::ModelTypeSet)> ready_task,
-    base::Callback<void()> retry_callback) {
-  syncable::ModelTypeSet types_to_add_with_nigori = types_to_add;
-  syncable::ModelTypeSet types_to_remove_with_nigori = types_to_remove;
+    const base::Callback<void(syncable::ModelTypeSet)>& ready_task,
+    const base::Callback<void()>& retry_callback) {
+  syncable::ModelTypeSet desired_types_with_nigori = desired_types;
+  syncable::ModelTypeSet disabled_types_with_nigori = disabled_types;
   if (nigori_state == WITH_NIGORI) {
-    types_to_add_with_nigori.Put(syncable::NIGORI);
-    types_to_remove_with_nigori.Remove(syncable::NIGORI);
+    desired_types_with_nigori.Put(syncable::NIGORI);
+    disabled_types_with_nigori.Remove(syncable::NIGORI);
   } else {
-    types_to_add_with_nigori.Remove(syncable::NIGORI);
-    types_to_remove_with_nigori.Put(syncable::NIGORI);
+    desired_types_with_nigori.Remove(syncable::NIGORI);
+    disabled_types_with_nigori.Put(syncable::NIGORI);
   }
-  // Only one configure is allowed at a time.
-  DCHECK(!pending_config_mode_state_.get());
-  DCHECK(!pending_download_state_.get());
+  // Only one configure is allowed at a time (DataTypeManager handles user
+  // changes that happen while the syncer is reconfiguraing, and will only
+  // trigger another call to ConfigureDataTypes once the current reconfiguration
+  // completes).
   DCHECK_GT(initialization_state_, NOT_INITIALIZED);
 
-  pending_config_mode_state_.reset(new PendingConfigureDataTypesState());
-  pending_config_mode_state_->ready_task = ready_task;
-  pending_config_mode_state_->types_to_add = types_to_add_with_nigori;
-  pending_config_mode_state_->added_types =
-      registrar_->ConfigureDataTypes(types_to_add_with_nigori,
-                                     types_to_remove_with_nigori);
-  pending_config_mode_state_->reason = reason;
-  pending_config_mode_state_->retry_callback = retry_callback;
-
-  // Cleanup disabled types before starting configuration so that
-  // callers can assume that the data types are cleaned up once
-  // configuration is done.
-  if (!types_to_remove_with_nigori.Empty()) {
-    ModelSafeRoutingInfo routing_info;
-    registrar_->GetModelSafeRoutingInfo(&routing_info);
-    sync_thread_.message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&SyncBackendHost::Core::DoRequestCleanupDisabledTypes,
-                   core_.get(),
-                   routing_info));
-  }
+  // enabled_types \setunion disabled_types gives the set of all registered
+  // types (i.e. types with datatype controllers registered).
+  ModelSafeRoutingInfo routing_info;
+  registrar_->ConfigureDataTypes(desired_types_with_nigori,
+                                 disabled_types_with_nigori);
+  registrar_->GetModelSafeRoutingInfo(&routing_info);
 
-  StartConfiguration(
-      base::Bind(&SyncBackendHost::Core::FinishConfigureDataTypes,
-                 core_.get()));
-}
+  // |enabled_types| is desired_types_with_nigori \setintersect types with
+  // model safe workers (e.g. the password store may fail to start up, in which
+  // case we have no model safe worker for it and should not enable it).
+  // TODO(sync): This shouldn't be necessary. We should do whatever filtering
+  // we need beforehand as part of ProfileSyncComponentsFactory's
+  // RegisterDataTypes.
+  const syncable::ModelTypeSet enabled_types =
+      GetRoutingInfoTypes(routing_info);
 
-void SyncBackendHost::StartConfiguration(const base::Closure& callback) {
-  // Put syncer in the config mode. DTM will put us in normal mode once it is
-  // done. This is to ensure we dont do a normal sync when we are doing model
-  // association.
-  sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
-      &SyncBackendHost::Core::DoStartConfiguration, core_.get(), callback));
+  // Figure out which types need to actually be downloaded. We pass those on
+  // to the syncer while it's in configuration mode so that they can be
+  // downloaded before we perform association. Once we switch to normal mode
+  // downloads will get applied normally and hit the datatype's change
+  // processor.
+  // A datatype is in need of downloading if any of the following are true:
+  // 1. it's enabled and initial_sync_ended is false (initial_sync_ended is
+  //    set after applying updates, and hence is a more conservative measure
+  //    than having a non-empty progress marker, which is set during
+  //    StoreTimestamps).
+  // 2. the type is NIGORI, and any other datatype is being configured (nigori
+  //    is always included if we do a configuration).

[tim (not reviewing), 2012/06/14 00:26:53]

This (and the logic below) seems a bit convoluted?  If we are downloading
anything at all, we must always download NIGORI. Unless there is explicit need
to disable NIGORI (e.g. for migration).  Mentioning 'configuration' here and
making it depend on 'if (types_to_config.Size() > 1)' makes it sound more
special cased than it is.

[Nicolas Zea, 2012/06/14 22:35:52]

I disagree that it's not already special cased :-). That said, what are you
suggesting? I've gone ahead and changed "configured" to "downloaded", to be more
accurate, but I think the logic below (barring the bug you found) is
straightforward?

+  // TODO(sync): consider moving this logic onto the sync thread (perhaps
+  // as part of SyncManager::ConfigureSyncer).
+  syncable::ModelTypeSet initial_sync_ended_types =
+      core_->sync_manager()->InitialSyncEndedTypes();
+  initial_sync_ended_types.RetainAll(enabled_types);
+  syncable::ModelTypeSet types_to_config =
+      Difference(enabled_types, initial_sync_ended_types);
+  if (types_to_config.Size() > 1 && enabled_types.Has(syncable::NIGORI))

[tim (not reviewing), 2012/06/14 00:26:53]

Your comment above says "any other data type is being configured" but you check
> 1 (and not > 0, or empty()). 

[Nicolas Zea, 2012/06/14 22:35:52]

Good catch, the logic was slightly different in a previous version and I didn't
update this appropriately. Done.

+    types_to_config.Put(syncable::NIGORI);
+
+  SDVLOG(1) << "Types "
+            << syncable::ModelTypeSetToString(types_to_config)
+            << " added; calling DoConfigureSyncer";
+  // TODO(zea): figure out how to bypass this call if no types are being
+  // configured and GetKey is not needed. For now we rely on determining the
+  // need for GetKey as part of the SyncManager::ConfigureSyncer logic.
+  RequestConfigureSyncer(reason,
+                         types_to_config,
+                         routing_info,
+                         ready_task,
+                         retry_callback);
 }
 
 void SyncBackendHost::EnableEncryptEverything() {
@@ -687,157 +703,47 @@ void SyncBackendHost::InitCore(const DoInitializeOptions& options) {
       base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options));
 }
 
-void SyncBackendHost::HandleSyncCycleCompletedOnFrontendLoop(
-    const SyncSessionSnapshot& snapshot) {
-  if (!frontend_)
-    return;
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-
-  last_snapshot_ = snapshot;
-
-  SDVLOG(1) << "Got snapshot " << snapshot.ToString();
-
-  const syncable::ModelTypeSet to_migrate =
-      snapshot.syncer_status().types_needing_local_migration;
-  if (!to_migrate.Empty())
-    frontend_->OnMigrationNeededForTypes(to_migrate);
-
-  // Process any changes to the datatypes we're syncing.
-  // TODO(sync): add support for removing types.
-  if (initialized())
-    AddExperimentalTypes();
-
-  // If we are waiting for a configuration change, check here to see
-  // if this sync cycle has initialized all of the types we've been
-  // waiting for.
-  if (pending_download_state_.get()) {
-    const syncable::ModelTypeSet types_to_add =
-        pending_download_state_->types_to_add;
-    const syncable::ModelTypeSet added_types =
-        pending_download_state_->added_types;
-    DCHECK(types_to_add.HasAll(added_types));
-    const syncable::ModelTypeSet initial_sync_ended =
-        snapshot.initial_sync_ended();
-    const syncable::ModelTypeSet failed_configuration_types =
-        Difference(added_types, initial_sync_ended);
-    SDVLOG(1)
-        << "Added types: "
-        << syncable::ModelTypeSetToString(added_types)
-        << ", configured types: "
-        << syncable::ModelTypeSetToString(initial_sync_ended)
-        << ", failed configuration types: "
-        << syncable::ModelTypeSetToString(failed_configuration_types);
-
-    if (!failed_configuration_types.Empty() &&
-        snapshot.retry_scheduled()) {
-      // Inform the caller that download failed but we are retrying.
-      if (!pending_download_state_->retry_in_progress) {
-        pending_download_state_->retry_callback.Run();
-        pending_download_state_->retry_in_progress = true;
-      }
-      // Nothing more to do.
-      return;
-    }
-
-    scoped_ptr<PendingConfigureDataTypesState> state(
-        pending_download_state_.release());
-    state->ready_task.Run(failed_configuration_types);
-
-    // Syncer did not report an error but did not download everything
-    // we requested either. So abort. The caller of the config will cleanup.
-    if (!failed_configuration_types.Empty())
-      return;
-  }
-
-  if (initialized())
-    frontend_->OnSyncCycleCompleted();
-}
-
-void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() {
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-  // Nudge the syncer. This is necessary for both datatype addition/deletion.
-  //
-  // Deletions need a nudge in order to ensure the deletion occurs in a timely
-  // manner (see issue 56416).
-  //
-  // In the case of additions, on the next sync cycle, the syncer should
-  // notice that the routing info has changed and start the process of
-  // downloading updates for newly added data types.  Once this is
-  // complete, the configure_state_.ready_task_ is run via an
-  // OnInitializationComplete notification.
-
-  SDVLOG(1) << "Syncer in config mode. SBH executing "
-            << "FinishConfigureDataTypesOnFrontendLoop";
-
-
-  ModelSafeRoutingInfo routing_info;
-  registrar_->GetModelSafeRoutingInfo(&routing_info);
-  const syncable::ModelTypeSet enabled_types =
-      GetRoutingInfoTypes(routing_info);
+void SyncBackendHost::RequestConfigureSyncer(
+    sync_api::ConfigureReason reason,
+    syncable::ModelTypeSet types_to_config,
+    const browser_sync::ModelSafeRoutingInfo& routing_info,
+    const base::Callback<void(syncable::ModelTypeSet)>& ready_task,
+    const base::Closure& retry_callback) {
+  sync_thread_.message_loop()->PostTask(FROM_HERE,
+       base::Bind(&SyncBackendHost::Core::DoConfigureSyncer,
+                  core_.get(),
+                  reason,
+                  types_to_config,
+                  routing_info,
+                  ready_task,
+                  retry_callback));
+}
+
+void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop(
+    const syncable::ModelTypeSet& types_to_configure,
+    const syncable::ModelTypeSet& configured_types,
+    const base::Callback<void(syncable::ModelTypeSet)>& ready_task) {
+  const syncable::ModelTypeSet failed_configuration_types =
+      Difference(types_to_configure, configured_types);
+  SDVLOG(1)
+      << "Added types: "
+      << syncable::ModelTypeSetToString(types_to_configure)
+      << ", configured types: "
+      << syncable::ModelTypeSetToString(configured_types)
+      << ", failed configuration types: "
+      << syncable::ModelTypeSetToString(failed_configuration_types);
 
   // Update |chrome_sync_notification_bridge_|'s enabled types here as it has
   // to happen on the UI thread.
-  chrome_sync_notification_bridge_.UpdateEnabledTypes(enabled_types);
-
-  if (pending_config_mode_state_->added_types.Empty() &&
-      !core_->sync_manager()->InitialSyncEndedTypes().HasAll(enabled_types)) {
-
-    // TODO(tim): Log / UMA / count this somehow?
-    // Add only the types with empty progress markers. Note: it is possible
-    // that some types have their initial_sync_ended be false but with non
-    // empty progress marker. Which is ok as the rest of the changes would
-    // be downloaded on a regular nudge and initial_sync_ended should be set
-    // to true. However this is a very corner case. So it is not explicitly
-    // handled.
-    pending_config_mode_state_->added_types =
-        sync_api::GetTypesWithEmptyProgressMarkerToken(enabled_types,
-                                                       GetUserShare());
-  }
-
-  // If we've added types, we always want to request a nudge/config (even if
-  // the initial sync is ended), in case we could not decrypt the data.
-  if (pending_config_mode_state_->added_types.Empty()) {
-    SDVLOG(1) << "No new types added; calling ready_task directly";
-    // No new types - just notify the caller that the types are available.
-    const syncable::ModelTypeSet failed_configuration_types;
-    pending_config_mode_state_->ready_task.Run(failed_configuration_types);
-  } else {
-    pending_download_state_.reset(pending_config_mode_state_.release());
-
-    // Always configure nigori if it's enabled.
-    syncable::ModelTypeSet types_to_config =
-        pending_download_state_->added_types;
-    if (IsNigoriEnabled()) {
-      // Note: Nigori is the only type that gets added with a nonempty
-      // progress marker during config. If the server returns a migration
-      // error then we will go into unrecoverable error. We dont handle it
-      // explicitly because server might help us out here by not sending a
-      // migraiton error for nigori during config.
-      types_to_config.Put(syncable::NIGORI);
-    }
-    SDVLOG(1) << "Types "
-              << syncable::ModelTypeSetToString(types_to_config)
-              << " added; calling DoRequestConfig";
-    ModelSafeRoutingInfo routing_info;
-    registrar_->GetModelSafeRoutingInfo(&routing_info);
-    sync_thread_.message_loop()->PostTask(FROM_HERE,
-         base::Bind(&SyncBackendHost::Core::DoRequestConfig,
-                    core_.get(),
-                    routing_info,
-                    types_to_config,
-                    pending_download_state_->reason));
-  }
-
-  pending_config_mode_state_.reset();
+  chrome_sync_notification_bridge_.UpdateEnabledTypes(configured_types);
 
   // Notify SyncManager (especially the notification listener) about new types.
   sync_thread_.message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoUpdateEnabledTypes, core_.get(),
-                 enabled_types));
-}
+                 configured_types));
 
-bool SyncBackendHost::IsDownloadingNigoriForTest() const {
-  return initialization_state_ == DOWNLOADING_NIGORI;
+  if (!ready_task.is_null())
+    ready_task.Run(failed_configuration_types);
 }
 
 SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
@@ -894,14 +800,6 @@ SyncBackendHost::Core::~Core() {
   DCHECK(!sync_loop_);
 }
 
-SyncBackendHost::PendingConfigureDataTypesState::
-PendingConfigureDataTypesState()
-    : reason(sync_api::CONFIGURE_REASON_UNKNOWN),
-      retry_in_progress(false) {}
-
-SyncBackendHost::PendingConfigureDataTypesState::
-~PendingConfigureDataTypesState() {}
-
 void SyncBackendHost::Core::OnSyncCycleCompleted(
     const SyncSessionSnapshot& snapshot) {
   if (!sync_loop_)
@@ -1153,12 +1051,6 @@ void SyncBackendHost::Core::DoRequestClearServerData() {
   sync_manager_->RequestClearServerData();
 }
 
-void SyncBackendHost::Core::DoRequestCleanupDisabledTypes(
-    const browser_sync::ModelSafeRoutingInfo& routing_info) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  sync_manager_->RequestCleanupDisabledTypes(routing_info);
-}
-
 void SyncBackendHost::Core::DoSetEncryptionPassphrase(
     const std::string& passphrase,
     bool is_explicit) {
@@ -1210,18 +1102,46 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   host_.Reset();
 }
 
-void SyncBackendHost::Core::DoRequestConfig(
-    const browser_sync::ModelSafeRoutingInfo& routing_info,
+void SyncBackendHost::Core::DoConfigureSyncer(
+    sync_api::ConfigureReason reason,
     syncable::ModelTypeSet types_to_config,
-    sync_api::ConfigureReason reason) {
+    const browser_sync::ModelSafeRoutingInfo routing_info,
+    const base::Callback<void(syncable::ModelTypeSet)>& ready_task,
+    const base::Closure& retry_callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  sync_manager_->RequestConfig(routing_info, types_to_config, reason);
+  sync_manager_->ConfigureSyncer(
+      reason,
+      types_to_config,
+      routing_info,
+      base::Bind(&SyncBackendHost::Core::DoFinishConfigureDataTypes,
+                 this,
+                 types_to_config,
+                 ready_task),
+      base::Bind(&SyncBackendHost::Core::DoRetryConfiguration,
+                 this,
+                 retry_callback));
 }
 
-void SyncBackendHost::Core::DoStartConfiguration(
-    const base::Closure& callback) {
+void SyncBackendHost::Core::DoFinishConfigureDataTypes(
+    syncable::ModelTypeSet types_to_config,
+    const base::Callback<void(syncable::ModelTypeSet)>& ready_task) {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  syncable::ModelTypeSet configured_types =
+      sync_manager_->InitialSyncEndedTypes();
+  configured_types.RetainAll(types_to_config);
+  host_.Call(FROM_HERE,
+             &SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop,
+             types_to_config,
+             configured_types,
+             ready_task);
+}
+
+void SyncBackendHost::Core::DoRetryConfiguration(
+    const base::Closure& retry_callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  sync_manager_->StartConfigurationMode(callback);
+  host_.Call(FROM_HERE,
+             &SyncBackendHost::RetryConfigurationOnFrontendLoop,
+             retry_callback);
 }
 
 void SyncBackendHost::Core::DeleteSyncDataFolder() {
@@ -1232,13 +1152,6 @@ void SyncBackendHost::Core::DeleteSyncDataFolder() {
   }
 }
 
-void SyncBackendHost::Core::FinishConfigureDataTypes() {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  host_.Call(
-      FROM_HERE,
-      &SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop);
-}
-
 void SyncBackendHost::Core::StartSavingChanges() {
   // We may already be shut down.
   if (!sync_loop_)
@@ -1289,14 +1202,6 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
     return;
   }
 
-  // If setup has completed, start off in DOWNLOADING_NIGORI so that
-  // we start off by refreshing nigori.
-  CHECK(sync_prefs_.get());
-  if (sync_prefs_->HasSyncSetupCompleted() &&
-      initialization_state_ < DOWNLOADING_NIGORI) {
-    initialization_state_ = DOWNLOADING_NIGORI;
-  }
-
   // Run initialization state machine.
   switch (initialization_state_) {
     case NOT_INITIALIZED:
@@ -1337,6 +1242,36 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
   }
 }
 
+void SyncBackendHost::HandleSyncCycleCompletedOnFrontendLoop(
+    const SyncSessionSnapshot& snapshot) {
+  if (!frontend_)
+    return;
+  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+
+  last_snapshot_ = snapshot;
+
+  SDVLOG(1) << "Got snapshot " << snapshot.ToString();
+
+  const syncable::ModelTypeSet to_migrate =
+      snapshot.syncer_status().types_needing_local_migration;
+  if (!to_migrate.Empty())
+    frontend_->OnMigrationNeededForTypes(to_migrate);
+
+  // Process any changes to the datatypes we're syncing.
+  // TODO(sync): add support for removing types.
+  if (initialized())
+    AddExperimentalTypes();
+
+  if (initialized())
+    frontend_->OnSyncCycleCompleted();
+}
+
+void SyncBackendHost::RetryConfigurationOnFrontendLoop(
+    const base::Closure& retry_callback) {
+  SDVLOG(1) << "Failed to complete configuration, informing of retry.";
+  retry_callback.Run();
+}
+
 void SyncBackendHost::PersistEncryptionBootstrapToken(
     const std::string& token) {
   CHECK(sync_prefs_.get());