[Sync] Make BackendMigrator not wait for full sync cycles

chrome/browser/sync/backend_migrator.cc

Index: chrome/browser/sync/backend_migrator.cc
diff --git a/chrome/browser/sync/backend_migrator.cc b/chrome/browser/sync/backend_migrator.cc
index cc0b613349a28490dd7553b9224028fee2e22e66..a7194900a7bbb4b3a3498ea600299818fd5ca6ee 100644
--- a/chrome/browser/sync/backend_migrator.cc
+++ b/chrome/browser/sync/backend_migrator.cc
@@ -6,13 +6,13 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
+#include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/engine/configure_reason.h"
-#include "chrome/browser/sync/glue/data_type_manager.h"
 #include "chrome/browser/sync/sessions/session_state.h"
 #include "chrome/common/chrome_notification_types.h"
-#include "content/browser/browser_thread.h"
 #include "content/common/notification_details.h"
 #include "content/common/notification_source.h"
 
@@ -21,46 +21,62 @@ using syncable::ModelTypeSet;
 namespace browser_sync {
 
 using sessions::SyncSessionSnapshot;
+using syncable::ModelTypeToString;
 
 BackendMigrator::BackendMigrator(ProfileSyncService* service,
                                  DataTypeManager* manager)
     : state_(IDLE), service_(service), manager_(manager),
-      restart_migration_(false),
-      method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   registrar_.Add(this, chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
                  Source<DataTypeManager>(manager_));
-  service_->AddObserver(this);
 }
 
 BackendMigrator::~BackendMigrator() {
-  service_->RemoveObserver(this);
-}
-
-bool BackendMigrator::HasStartedMigrating() const {
-  return state_ >= DISABLING_TYPES;
 }
 
 void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
+  const ModelTypeSet old_to_migrate = to_migrate_;
   {
+    using std::swap;

[tim (not reviewing), 2011/08/22 15:08:49]

nit - I'd just specify std::swap inline since you only use it once...

[akalin, 2011/08/24 22:50:17]

Done.

     ModelTypeSet temp;
     std::set_union(to_migrate_.begin(), to_migrate_.end(),
                    types.begin(), types.end(),
                    std::inserter(temp, temp.end()));
-    to_migrate_ = temp;
+    swap(temp, to_migrate_);
   }
-
-  if (HasStartedMigrating()) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: STARTED_MIGRATING early-out.";
-    restart_migration_ = true;
+  VLOG(1) << "MigrateTypes called with " << ModelTypeSetToString(types)
+          << ", old_to_migrate = " << ModelTypeSetToString(old_to_migrate)
+          << ", to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (old_to_migrate == to_migrate_) {
+    VLOG(1) << "MigrateTypes called with no new types; ignoring";
     return;
   }
 
-  if (manager_->state() != DataTypeManager::CONFIGURED) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: manager CONFIGURED early-out.";
+  if (state_ == IDLE) {

[tim (not reviewing), 2011/08/22 15:08:49]

nit - file uses no-braces-on-single-line-if

[akalin, 2011/08/24 22:50:17]

Done.

     state_ = WAITING_TO_START;
+  }
+
+  if (state_ == WAITING_TO_START) {
+    TryStart();

[tim (not reviewing), 2011/08/22 15:08:49]

Would probably be simpler (here and below) if TryStart returned a bool. Makes
sense too given that it's named 'TryStart'.

[akalin, 2011/08/24 22:50:17]

Done.

+    if (state_ == WAITING_TO_START) {
+      VLOG(1) << "Manager not configured; waiting";
+    }
     return;
   }
 
+  DCHECK_GT(state_, WAITING_TO_START);
+  // If we're already migrating, interrupt the current migration.
+  RestartMigration();
+}
+
+void BackendMigrator::TryStart() {
+  DCHECK_EQ(state_, WAITING_TO_START);
+  if (manager_->state() == DataTypeManager::CONFIGURED) {
+    RestartMigration();
+  }
+}
+
+void BackendMigrator::RestartMigration() {
   // We'll now disable any running types that need to be migrated.
   state_ = DISABLING_TYPES;
   ModelTypeSet full_set;
@@ -69,11 +85,15 @@ void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
   std::set_difference(full_set.begin(), full_set.end(),
                       to_migrate_.begin(), to_migrate_.end(),
                       std::inserter(difference, difference.end()));
-  VLOG(1) << "BackendMigrator disabling types; calling Configure.";
+  bool configure_with_nigori = to_migrate_.count(syncable::NIGORI) == 0;
+  VLOG(1) << "BackendMigrator disabling types "
+          << ModelTypeSetToString(to_migrate_) << "; configuring "
+          << ModelTypeSetToString(difference)
+          << (configure_with_nigori ? " with nigori" : " without nigori");
 
   // Add nigori for config or not based upon if the server told us to migrate
   // nigori or not.
-  if (to_migrate_.count(syncable::NIGORI) == 0) {
+  if (configure_with_nigori) {
     manager_->Configure(difference, sync_api::CONFIGURE_REASON_MIGRATION);
   } else {
     manager_->ConfigureWithoutNigori(difference,
@@ -81,39 +101,6 @@ void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
   }
 }
 
-void BackendMigrator::OnStateChanged() {
-  if (restart_migration_ == true) {
-    VLOG(1) << "BackendMigrator restarting migration in OnStateChanged.";
-    state_ = WAITING_TO_START;
-    restart_migration_ = false;
-    MigrateTypes(to_migrate_);
-    return;
-  }
-
-  if (state_ != WAITING_FOR_PURGE)
-    return;
-
-  size_t num_empty_migrated_markers = 0;
-  const SyncSessionSnapshot* snap = service_->GetLastSessionSnapshot();
-  for (ModelTypeSet::const_iterator it = to_migrate_.begin();
-       it != to_migrate_.end(); ++it) {
-    if (snap->download_progress_markers[*it].empty())
-      num_empty_migrated_markers++;
-  }
-
-  if (num_empty_migrated_markers < to_migrate_.size())

[tim (not reviewing), 2011/08/22 15:08:49]

Is there a way we can still have this check?  I know it's not needed in the
happy case, but attempting to re-sync a datatype that hasn't been purged is
quite bad and leaves the whole account in an unspecified state...  Just before
configuring to re-enable, it would be good to make sure the clear took place. 
It's better to "do nothing" than try and reconfigure.

[akalin, 2011/08/24 22:55:01]

Done.

-    return;
-
-  state_ = REENABLING_TYPES;
-  ModelTypeSet full_set;
-  service_->GetPreferredDataTypes(&full_set);
-  VLOG(1) << "BackendMigrator re-enabling types.";
-
-  // Don't use |to_migrate_| for the re-enabling because the user may have
-  // chosen to disable types during the migration.
-  manager_->Configure(full_set, sync_api::CONFIGURE_REASON_MIGRATION);
-}
-
 void BackendMigrator::Observe(int type,
                               const NotificationSource& source,
                               const NotificationDetails& details) {
@@ -121,36 +108,46 @@ void BackendMigrator::Observe(int type,
   if (state_ == IDLE)
     return;
 
-  const DataTypeManager::ConfigureResult* result =
-      Details<DataTypeManager::ConfigureResult>(details).ptr();
+  // |manager_|'s methods aren't re-entrant, and we're notified from
+  // them, so post a task to avoid problems.
+  VLOG(1) << "Posting OnConfigureDone from Observer";
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&BackendMigrator::OnConfigureDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 *Details<DataTypeManager::ConfigureResult>(details).ptr()));
+}
+
+void BackendMigrator::OnConfigureDone(
+    const DataTypeManager::ConfigureResult& result) {
+  VLOG(1) << "OnConfigureDone with types "
+          << ModelTypeSetToString(result.requested_types)
+          << ", status " << result.status
+          << ", and to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (state_ == WAITING_TO_START) {
+    TryStart();
+    if (state_ == WAITING_TO_START) {
+      VLOG(1) << "Manager still not configured; still waiting";
+    }
+    return;
+  }
+
+  DCHECK_GT(state_, WAITING_TO_START);
 
   ModelTypeSet intersection;
-  std::set_intersection(result->requested_types.begin(),
-      result->requested_types.end(), to_migrate_.begin(), to_migrate_.end(),
+  std::set_intersection(
+      result.requested_types.begin(), result.requested_types.end(),
+      to_migrate_.begin(), to_migrate_.end(),
       std::inserter(intersection, intersection.end()));
-
-  // The intersection check is to determine if our disable request was
-  // interrupted by a user changing preferred types.  May still need to purge.
-  // It's pretty wild if we're in WAITING_FOR_PURGE here, because it would mean
-  // that after our disable-config finished but before the purge, another config
-  // was posted externally _and completed_, which means somehow the nudge to
-  // purge was dropped, yet nudges are reliable.
-  if (state_ == WAITING_TO_START || state_ == WAITING_FOR_PURGE ||
-      (state_ == DISABLING_TYPES && !intersection.empty())) {
-    state_ = WAITING_TO_START;
-    restart_migration_ = false;
-    VLOG(1) << "BackendMigrator::Observe posting MigrateTypes.";
-    if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-        method_factory_.NewRunnableMethod(&BackendMigrator::MigrateTypes,
-                                          to_migrate_))) {
-      // Unittests need this.
-      // TODO(tim): Clean this up.
-      MigrateTypes(to_migrate_);
-    }
+  // This intersection check is to determine if our disable request
+  // was interrupted by a user changing preferred types.
+  if (state_ == DISABLING_TYPES && !intersection.empty()) {
+    VLOG(1) << "Disable request interrupted by user changing types";
+    RestartMigration();
     return;
   }
 
-  if (result->status != DataTypeManager::OK) {
+  if (result.status != DataTypeManager::OK) {
     // If this fails, and we're disabling types, a type may or may not be
     // disabled until the user restarts the browser.  If this wasn't an abort,
     // any failure will be reported as an unrecoverable error to the UI. If it
@@ -165,16 +162,20 @@ void BackendMigrator::Observe(int type,
   }
 
   if (state_ == DISABLING_TYPES) {
-    state_ = WAITING_FOR_PURGE;
-    VLOG(1) << "BackendMigrator waiting for purge.";
+    state_ = REENABLING_TYPES;
+    // Don't use |to_migrate_| for the re-enabling because the user
+    // may have chosen to disable types during the migration.
+    ModelTypeSet full_set;
+    service_->GetPreferredDataTypes(&full_set);
+    VLOG(1) << "BackendMigrator re-enabling types: "
+            << syncable::ModelTypeSetToString(full_set);
+    manager_->Configure(full_set, sync_api::CONFIGURE_REASON_MIGRATION);
   } else if (state_ == REENABLING_TYPES) {
     // We're done!
     state_ = IDLE;
 
-    std::stringstream ss;
-    std::copy(to_migrate_.begin(), to_migrate_.end(),
-              std::ostream_iterator<syncable::ModelType>(ss, ","));
-    VLOG(1) << "BackendMigrator: Migration complete for: " << ss.str();
+    VLOG(1) << "BackendMigrator: Migration complete for: "
+            << syncable::ModelTypeSetToString(to_migrate_);
     to_migrate_.clear();
   }
 }