[Sync] Refactor sync configuration logic.

sync/internal_api/sync_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/internal_api/sync_manager.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 40 matching lines @@
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/directory_change_delegate.h"
 #include "sync/syncable/syncable.h"
 #include "sync/util/cryptographer.h"
 #include "sync/util/experiments.h"
 #include "sync/util/get_session_name.h"
 #include "sync/util/time.h"
 
 using base::TimeDelta;
 using browser_sync::AllStatus;
+using browser_sync::ConfigureParams;
 using browser_sync::Cryptographer;
 using browser_sync::Encryptor;
 using browser_sync::JsArgList;
 using browser_sync::JsBackend;
 using browser_sync::JsEventDetails;
 using browser_sync::JsEventHandler;
 using browser_sync::JsEventHandler;
 using browser_sync::JsReplyHandler;
 using browser_sync::JsMutationEventObserver;
 using browser_sync::JsSyncManagerObserver;
@@ skipping 764 matching lines @@
 
 bool SyncManager::EncryptEverythingEnabledForTest() const {
   ReadTransaction trans(FROM_HERE, GetUserShare());
   return trans.GetCryptographer()->encrypt_everything();
 }
 
 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());
+  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();
+}
+
+bool SyncManager::DoConfigureSyncer(const ConfigureParams& params) {
   if (!data_->scheduler()) {
     LOG(INFO)
-        << "SyncManager::RequestConfig: bailing out because scheduler is "
-        << "null";
-    return;
+        << "SyncManager::DoConfigureSyncer: could not configure because "
+        << "scheduler is null";
+    params.ready_task.Run();
+    return true;
   }
-  StartConfigurationMode(base::Closure());
-  data_->session_context()->set_routing_info(routing_info);
-  data_->scheduler()->ScheduleConfiguration(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;
-  }
-  data_->scheduler()->Start(
-      browser_sync::SyncScheduler::CONFIGURATION_MODE, callback);
+  data_->scheduler()->Start(SyncScheduler::CONFIGURATION_MODE);
+  return data_->scheduler()->ScheduleConfiguration(params);
 }
 
 bool SyncManager::SyncInternal::Init(
     const FilePath& database_location,
     const WeakHandle<JsEventHandler>& event_handler,
     const std::string& sync_server_and_path,
     int port,
     bool use_ssl,
     const scoped_refptr<base::TaskRunner>& blocking_task_runner,
     HttpPostProviderFactory* post_factory,
@@ skipping 64 matching lines @@
         &debug_info_event_listener_,
         &traffic_recorder_));
     session_context()->set_account_name(credentials.email);
     scheduler_.reset(new SyncScheduler(name_, session_context(), new Syncer()));
   }
 
   bool signed_in = SignIn(credentials);
 
   if (signed_in) {
     if (scheduler()) {
-      scheduler()->Start(
-          browser_sync::SyncScheduler::CONFIGURATION_MODE, base::Closure());
+      scheduler()->Start(SyncScheduler::CONFIGURATION_MODE);
     }
 
     initialized_ = true;
 
     // Cryptographer should only be accessed while holding a
     // transaction.  Grabbing the user share for the transaction
     // checks the initialization state, so this must come after
     // |initialized_| is set to true.
     ReadTransaction trans(FROM_HERE, GetUserShare());
     trans.GetCryptographer()->Bootstrap(restored_key_for_bootstrapping);
@@ skipping 153 matching lines @@
   allstatus_.SetCryptographerReady(cryptographer->is_ready());
   allstatus_.SetCryptoHasPendingKeys(cryptographer->has_pending_keys());
   debug_info_event_listener_.SetCryptographerReady(cryptographer->is_ready());
   debug_info_event_listener_.SetCrytographerHasPendingKeys(
       cryptographer->has_pending_keys());
 }
 
 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);
   }
 }
 
 bool SyncManager::SyncInternal::OpenDirectory() {
   DCHECK(!initialized_) << "Should only happen once";
 
   // Set before Open().
   change_observer_ =
       browser_sync::MakeWeakHandle(js_mutation_event_observer_.AsWeakPtr());
   WeakHandle<syncable::TransactionObserver> transaction_observer(
@@ skipping 1345 matching lines @@
     share->directory->GetDownloadProgress(i.Get(), &marker);
 
     if (marker.token().empty())
       result.Put(i.Get());
 
   }
   return result;
 }
 
 }  // namespace sync_api