[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 5921739ccfd933245f226819ded818745c50085d..4c170478660f0501e1e009d8c4242b276edbf9be 100644
--- a/sync/internal_api/sync_manager.cc
+++ b/sync/internal_api/sync_manager.cc
@@ -58,6 +58,7 @@
 
 using base::TimeDelta;
 using browser_sync::AllStatus;
+using browser_sync::ConfigureParams;
 using browser_sync::Cryptographer;
 using browser_sync::Encryptor;
 using browser_sync::JsArgList;
@@ -842,46 +843,49 @@ 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()->CleanupDisabledTypes();
-  }
-}
-
 void SyncManager::RequestClearServerData() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (data_->scheduler())
     data_->scheduler()->ClearUserData();
 }
 
-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()->ScheduleConfiguration(types, GetSourceFromReason(reason));
+  DCHECK(!ready_task.is_null());
+  DCHECK(!retry_task.is_null());
+
+  // TODO(zea): set this based on whether cryptographer has keystore

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

bug?

Also... dare I say it... enum vs bool?

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

:-/ Done to both.

+  // encryption key or not (requires opening a transaction).
+  bool need_encryption_key = false;
+
+  ConfigureParams params(GetSourceFromReason(reason),
+                         types_to_config,
+                         new_routing_info,
+                         need_encryption_key,
+                         ready_task);
+
+  // Returns false if a retry has been scheduled. Either way, will invoke the
+  // ready task when the configure job completes.
+  if (!DoConfigureSyncer(params))
+    retry_task.Run();
 }
 
-void SyncManager::StartConfigurationMode(const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+bool SyncManager::DoConfigureSyncer(const ConfigureParams& params) {
   if (!data_->scheduler()) {
     LOG(INFO)
-        << "SyncManager::StartConfigurationMode: could not start "
-        << "configuration mode because because scheduler is null";
-    return;
+        << "SyncManager::DoConfigureSyncer: could not configure because "
+        << "scheduler is null";
+    params.ready_task.Run();
+    return true;
   }
-  data_->scheduler()->Start(
-      browser_sync::SyncScheduler::CONFIGURATION_MODE, callback);
+
+  data_->scheduler()->Start(SyncScheduler::CONFIGURATION_MODE);
+  return data_->scheduler()->ScheduleConfiguration(params);
 }
 
 bool SyncManager::SyncInternal::Init(
@@ -966,8 +970,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;
@@ -1141,9 +1144,13 @@ void SyncManager::SyncInternal::NotifyCryptographerState(
 void SyncManager::SyncInternal::StartSyncingNormally(
     const browser_sync::ModelSafeRoutingInfo& routing_info) {
   // Start the sync scheduler.
-  if (scheduler()) { // NULL during certain unittests.
+  if (scheduler()) {  // NULL during certain unittests.
+    // TODO(sync): We always want the newest set of routes when we switch back
+    // to normal mode. Figure out how to enforce set_routing_info is always

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

Hmm... what do you want to figure out here, exactly?  

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

Eventually, I'd like to see the updating of the context's routing info happen
automatically within the scheduler, and not require the caller to remember to
set it properly. We only really need to set it when we call
Start(NORMAL_MODE)...

[tim (not reviewing), 2012/06/15 14:59:36]

I agree it's clunky to have to remember to set it :/  In the old registrar world
the scheduler and friends would take care of auto updating the routing info (by
grabbing a copy from the registrar).  What are you proposing?  (This doesn't
matter for this review, I was just curious)

[Nicolas Zea, 2012/06/15 21:23:36]

I think the simplest option might be passing the routing info in via Start(..).
Then perhaps making the set_routing_info method be private and friend the
scheduler? Not a big fan of friending, but I'd like to make it clear it should
only be set within the scheduler.

[tim (not reviewing), 2012/06/15 21:58:24]

OK. But I think there are friendless options :)  I proposed moving the context_
ownership from scheduler to SyncManager as part of the registrar-ectomy
precisely so that the SyncManager could set the routing info.  I would rather
not have an explicit setter on the scheduler for the context, but if it's
coupled to some other method (and can't change without another call to said
method / switching modes), we could move ownership back, meaning no one else
would have access to it.

I'm sure we can come up with something sufficiently elegant.  

+    // appropriately set and that it's only modified when switching to normal
+    // mode.
     session_context()->set_routing_info(routing_info);
-    scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure());
+    scheduler()->Start(SyncScheduler::NORMAL_MODE);
   }
 }