[Sync] Refactor sync configuration logic.

sync/internal_api/sync_manager.cc

Index: sync/internal_api/sync_manager.cc
diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc
index b25c88483e3f2a0a9174e07153f8b3b4635154c9..c7127866c5245aee172c5d399ac50b8ed37321b5 100644
--- a/sync/internal_api/sync_manager.cc
+++ b/sync/internal_api/sync_manager.cc
@@ -841,46 +841,60 @@ bool SyncManager::IsUsingExplicitPassphrase() {
   return data_ && data_->IsUsingExplicitPassphrase();
 }
 
-void SyncManager::RequestCleanupDisabledTypes(
-    const browser_sync::ModelSafeRoutingInfo& routing_info) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (data_->scheduler()) {
-    data_->session_context()->set_routing_info(routing_info);
-    data_->scheduler()->ScheduleCleanupDisabledTypes();
-  }
-}
-
 void SyncManager::RequestClearServerData() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (data_->scheduler())
     data_->scheduler()->ScheduleClearUserData();
 }
 
-void SyncManager::RequestConfig(
-    const browser_sync::ModelSafeRoutingInfo& routing_info,
-    const ModelTypeSet& types, ConfigureReason reason) {
+void SyncManager::ConfigureSyncer(
+    ConfigureReason reason,
+    const syncable::ModelTypeSet& types_to_config,
+    const browser_sync::ModelSafeRoutingInfo& new_routing_info,
+    const base::Closure& ready_task,
+    const base::Closure& retry_task) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!data_->scheduler()) {
-    LOG(INFO)
-        << "SyncManager::RequestConfig: bailing out because scheduler is "
-        << "null";
-    return;
-  }
-  StartConfigurationMode(base::Closure());
-  data_->session_context()->set_routing_info(routing_info);
-  data_->scheduler()->ScheduleConfig(types, GetSourceFromReason(reason));
-}
-
-void SyncManager::StartConfigurationMode(const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!data_->scheduler()) {
-    LOG(INFO)
-        << "SyncManager::StartConfigurationMode: could not start "
-        << "configuration mode because because scheduler is null";
-    return;
+  DCHECK(!ready_task.is_null());
+  DCHECK(!retry_task.is_null());
+  if (data_->scheduler()) {
+    // TODO(zea): set this based on whether cryptographer has keystore 
+    // encryption key or not (requires opening a transaction).
+    bool need_encryption_key = false;
+
+    // We do a cleanup if this is the first configuration after a restart (to
+    // handle the case where we failed to configure a previous type) or if we're
+    // disabling a type.
+    // Note: it's not necessary to check for empty progress markers. Any type
+    // that we attempted to enable would have been in the current routing info.
+    // If it failed to download, we'll either disable it, in which case it's
+    // removed from the routing info, or we attempt to retry it, in which case
+    // it would remain in the routing info (and since it didn't have
+    // initial_sync_ended set we'll ensure we attempt to redownload).
+    bool need_cleanup = false;
+    const browser_sync::ModelSafeRoutingInfo& current_routing_info =
+        data_->session_context()->routing_info();
+    if (current_routing_info.empty() ||                  // Restart case.

[rlarocque, 2012/06/04 23:01:38]

See CleanupDisabltedTypesCommand::ExecuteImpl().  We shouldn't need to have this
logic in both places.

[Nicolas Zea, 2012/06/07 19:25:42]

See other comment. This is gone now.

+        !GetRoutingInfoTypes(new_routing_info).HasAll(
+            GetRoutingInfoTypes(current_routing_info)))  // Disabled datatypes.
+      need_cleanup = true;
+
+    SyncScheduler::ConfigureParams params(
+        GetSourceFromReason(reason),
+        types_to_config,
+        new_routing_info,
+        need_cleanup,
+        need_encryption_key,
+        ready_task);
+    data_->scheduler()->Start(SyncScheduler::CONFIGURATION_MODE);
+    // Returns false if a retry has been scheduled. Either way, will invoke the
+    // ready task when the configure job completes.
+    if (!data_->scheduler()->Configure(params))
+      retry_task.Run();
+  } else {
+    LOG(INFO) << "SyncManager::ConfigureSyncer: bailing out because scheduler "
+              << "is null.";
+    ready_task.Run();
   }
-  data_->scheduler()->Start(
-      browser_sync::SyncScheduler::CONFIGURATION_MODE, callback);
 }
 
 bool SyncManager::SyncInternal::Init(
@@ -965,8 +979,7 @@ bool SyncManager::SyncInternal::Init(
 
   if (signed_in) {
     if (scheduler()) {
-      scheduler()->Start(
-          browser_sync::SyncScheduler::CONFIGURATION_MODE, base::Closure());
+      scheduler()->Start(SyncScheduler::CONFIGURATION_MODE);
     }
 
     initialized_ = true;
@@ -1140,10 +1153,8 @@ void SyncManager::SyncInternal::NotifyCryptographerState(
 void SyncManager::SyncInternal::StartSyncingNormally(
     const browser_sync::ModelSafeRoutingInfo& routing_info) {
   // Start the sync scheduler.
-  if (scheduler()) { // NULL during certain unittests.
-    session_context()->set_routing_info(routing_info);
-    scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure());
-  }
+  if (scheduler())  // NULL during certain unittests.
+    scheduler()->Start(SyncScheduler::NORMAL_MODE);
 }
 
 bool SyncManager::SyncInternal::OpenDirectory() {