[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 0c7c15ca039a4d3722997c0874aaccad9fd5c9e0..47fb4d67570f863f910f7e4aa91ae0140988ed8e 100644
--- a/chrome/browser/sync/backend_migrator.cc
+++ b/chrome/browser/sync/backend_migrator.cc
@@ -6,13 +6,16 @@
 
 #include <algorithm>
 
+#include "base/message_loop.h"
 #include "base/string_number_conversions.h"
-#include "chrome/browser/sync/glue/data_type_manager.h"
+#include "base/tracked_objects.h"
 #include "chrome/browser/sync/internal_api/configure_reason.h"
+#include "chrome/browser/sync/internal_api/read_transaction.h"
 #include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/sessions/session_state.h"
+#include "chrome/browser/sync/syncable/directory_manager.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,59 +24,108 @@ using syncable::ModelTypeSet;
 namespace browser_sync {
 
 using sessions::SyncSessionSnapshot;
+using syncable::ModelTypeToString;
 
-BackendMigrator::BackendMigrator(ProfileSyncService* service,
+MigrationObserver::~MigrationObserver() {}
+
+BackendMigrator::BackendMigrator(const std::string& name,
+                                 sync_api::UserShare* user_share,
+                                 ProfileSyncService* service,
                                  DataTypeManager* manager)
-    : state_(IDLE), service_(service), manager_(manager),
-      restart_migration_(false),
-      method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+    : name_(name), user_share_(user_share), service_(service),
+      manager_(manager), state_(IDLE),
+      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;
-}
+// Helper macros to log with the syncer thread name; useful when there
+// are multiple syncer threads involved.
+
+#define SLOG(severity) LOG(severity) << name_ << ": "
+
+#define SVLOG(verbose_level) VLOG(verbose_level) << name_ << ": "
 
 void BackendMigrator::MigrateTypes(const syncable::ModelTypeSet& types) {
+  const ModelTypeSet old_to_migrate = to_migrate_;
   {
     ModelTypeSet temp;
     std::set_union(to_migrate_.begin(), to_migrate_.end(),
                    types.begin(), types.end(),
                    std::inserter(temp, temp.end()));
-    to_migrate_ = temp;
+    std::swap(temp, to_migrate_);
   }
-
-  if (HasStartedMigrating()) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: STARTED_MIGRATING early-out.";
-    restart_migration_ = true;
+  SVLOG(1) << "MigrateTypes called with " << ModelTypeSetToString(types)
+           << ", old_to_migrate = " << ModelTypeSetToString(old_to_migrate)
+          << ", to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (old_to_migrate == to_migrate_) {
+    SVLOG(1) << "MigrateTypes called with no new types; ignoring";
     return;
   }
 
-  if (manager_->state() != DataTypeManager::CONFIGURED) {
-    VLOG(1) << "BackendMigrator::MigrateTypes: manager CONFIGURED early-out.";
-    state_ = WAITING_TO_START;
+  if (state_ == IDLE)
+    ChangeState(WAITING_TO_START);
+
+  if (state_ == WAITING_TO_START) {
+    if (!TryStart())
+      SVLOG(1) << "Manager not configured; waiting";
     return;
   }
 
+  DCHECK_GT(state_, WAITING_TO_START);
+  // If we're already migrating, interrupt the current migration.
+  RestartMigration();
+}
+
+void BackendMigrator::AddMigrationObserver(MigrationObserver* observer) {
+  migration_observers_.AddObserver(observer);
+}
+
+bool BackendMigrator::HasMigrationObserver(
+    MigrationObserver* observer) const {
+  return migration_observers_.HasObserver(observer);
+}
+
+void BackendMigrator::RemoveMigrationObserver(MigrationObserver* observer) {
+  migration_observers_.RemoveObserver(observer);
+}
+
+void BackendMigrator::ChangeState(State new_state) {
+  state_ = new_state;
+  FOR_EACH_OBSERVER(MigrationObserver, migration_observers_,
+                    OnMigrationStateChange());
+}
+
+bool BackendMigrator::TryStart() {
+  DCHECK_EQ(state_, WAITING_TO_START);
+  if (manager_->state() == DataTypeManager::CONFIGURED) {
+    RestartMigration();
+    return true;
+  }
+  return false;
+}
+
+void BackendMigrator::RestartMigration() {
   // We'll now disable any running types that need to be migrated.
-  state_ = DISABLING_TYPES;
+  ChangeState(DISABLING_TYPES);
   ModelTypeSet full_set;
   service_->GetPreferredDataTypes(&full_set);
   ModelTypeSet difference;
   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;
+  SVLOG(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,76 +133,70 @@ 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)
+void BackendMigrator::Observe(int type,
+                              const NotificationSource& source,
+                              const NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, type);
+  if (state_ == IDLE)
     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++;
-  }
+  // |manager_|'s methods aren't re-entrant, and we're notified from
+  // them, so post a task to avoid problems.
+  SVLOG(1) << "Posting OnConfigureDone from Observer";
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&BackendMigrator::OnConfigureDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 *Details<DataTypeManager::ConfigureResult>(details).ptr()));
+}
 
-  if (num_empty_migrated_markers < to_migrate_.size())
-    return;
+namespace {
 
-  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);
+syncable::ModelTypeSet GetUnsyncedDataTypes(sync_api::UserShare* user_share) {
+  sync_api::ReadTransaction trans(FROM_HERE, user_share);
+  syncable::ModelTypeSet unsynced_data_types;
+  for (int i = syncable::FIRST_REAL_MODEL_TYPE;
+       i < syncable::MODEL_TYPE_COUNT; ++i) {
+    syncable::ModelType type = syncable::ModelTypeFromInt(i);
+    sync_pb::DataTypeProgressMarker progress_marker;
+    trans.GetLookup()->GetDownloadProgress(type, &progress_marker);
+    if (progress_marker.token().empty()) {
+      unsynced_data_types.insert(type);
+    }
+  }
+  return unsynced_data_types;
 }
 
-void BackendMigrator::Observe(int type,
-                              const NotificationSource& source,
-                              const NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, type);
-  if (state_ == IDLE)
+}  // namespace
+
+void BackendMigrator::OnConfigureDone(
+    const DataTypeManager::ConfigureResult& result) {
+  SVLOG(1) << "OnConfigureDone with requested types "
+           << ModelTypeSetToString(result.requested_types)
+           << ", status " << result.status
+           << ", and to_migrate_ = " << ModelTypeSetToString(to_migrate_);
+  if (state_ == WAITING_TO_START) {
+    if (!TryStart())
+      SVLOG(1) << "Manager still not configured; still waiting";
     return;
+  }
 
-  const DataTypeManager::ConfigureResult* result =
-      Details<DataTypeManager::ConfigureResult>(details).ptr();
+  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()) {
+    SVLOG(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
@@ -158,23 +204,39 @@ void BackendMigrator::Observe(int type,
     // much we can do in any case besides wait until a restart to try again.
     // The server will send down MIGRATION_DONE again for types needing
     // migration as the type will still be enabled on restart.
-    LOG(WARNING) << "Unable to migrate, configuration failed!";
-    state_ = IDLE;
+    SLOG(WARNING) << "Unable to migrate, configuration failed!";
+    ChangeState(IDLE);
     to_migrate_.clear();
     return;
   }
 
   if (state_ == DISABLING_TYPES) {
-    state_ = WAITING_FOR_PURGE;
-    VLOG(1) << "BackendMigrator waiting for purge.";
+    const syncable::ModelTypeSet& unsynced_types =
+        GetUnsyncedDataTypes(user_share_);
+    if (!std::includes(unsynced_types.begin(), unsynced_types.end(),
+                       to_migrate_.begin(), to_migrate_.end())) {
+      SLOG(WARNING) << "Set of unsynced types: "
+                    << syncable::ModelTypeSetToString(unsynced_types)
+                    << " does not contain types to migrate: "
+                    << syncable::ModelTypeSetToString(to_migrate_)
+                    << "; not re-enabling yet";
+      return;
+    }
+
+    ChangeState(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);
+    SVLOG(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;
+    ChangeState(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();
+    SVLOG(1) << "BackendMigrator: Migration complete for: "
+             << syncable::ModelTypeSetToString(to_migrate_);
     to_migrate_.clear();
   }
 }
@@ -183,4 +245,13 @@ BackendMigrator::State BackendMigrator::state() const {
   return state_;
 }
 
+syncable::ModelTypeSet
+    BackendMigrator::GetPendingMigrationTypesForTest() const {
+  return to_migrate_;
+}
+
+#undef SVLOG
+
+#undef SLOG
+
 };  // namespace browser_sync