3 files changed, 71 insertions, 262 deletions

diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index d31888c6..75e55aa 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -31,7 +31,6 @@ struct Experiments;
 class ExtensionsActivityMonitor;
 class JsBackend;
 class JsEventHandler;
-class SyncScheduler;
 
 namespace sessions {
 class SyncSessionSnapshot;
@@ -402,16 +401,6 @@ class SyncManager {
   // Returns the set of types for which we have stored some sync data.
   syncer::ModelTypeSet InitialSyncEndedTypes();
 
-  // Returns those types within |types| that have an empty progress marker
-  // token.
-  syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
-      syncer::ModelTypeSet types);
-
-  // Purge from the directory those types with non-empty progress markers
-  // but without initial synced ended set.
-  // Returns false if an error occurred, true otherwise.
-  bool PurgePartiallySyncedTypes();
-
   // Update tokens that we're using in Sync. Email must stay the same.
   void UpdateCredentials(const SyncCredentials& credentials);
 
@@ -439,20 +428,20 @@ class SyncManager {
   // error to call this when we don't have pending keys.
   void SetDecryptionPassphrase(const std::string& passphrase);
 
-  // Switches the mode of operation to CONFIGURATION_MODE and performs
-  // any configuration tasks needed as determined by the params. Once complete,
-  // syncer will remain in CONFIGURATION_MODE until StartSyncingNormally is
-  // called.
-  // |ready_task| is invoked when the configuration completes.
-  // |retry_task| is invoked if the configuration job could not immediately
-  //              execute. |ready_task| will still be called when it eventually
-  //              does finish.
-  void ConfigureSyncer(
-      ConfigureReason reason,
-      const syncer::ModelTypeSet& types_to_config,
-      const syncer::ModelSafeRoutingInfo& new_routing_info,
-      const base::Closure& ready_task,
-      const base::Closure& retry_task);
+  // Puts the SyncScheduler into a mode where no normal nudge or poll traffic
+  // will occur, but calls to RequestConfig will be supported.  If |callback|
+  // is provided, it will be invoked (from the internal SyncScheduler) when
+  // the thread has changed to configuration mode.
+  void StartConfigurationMode(const base::Closure& callback);
+
+  // Switches the mode of operation to CONFIGURATION_MODE and
+  // schedules a config task to fetch updates for |types|.
+  void RequestConfig(const syncer::ModelSafeRoutingInfo& routing_info,
+                     const syncer::ModelTypeSet& types,
+                     syncer::ConfigureReason reason);
+
+  void RequestCleanupDisabledTypes(
+      const syncer::ModelSafeRoutingInfo& routing_info);
 
   // Adds a listener to be notified of sync events.
   // NOTE: It is OK (in fact, it's probably a good idea) to call this before
@@ -549,17 +538,11 @@ class SyncManager {
   static const FilePath::CharType kSyncDatabaseFilename[];
 
  private:
-  friend class SyncManagerTest;
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, NudgeDelayTest);
 
   // For unit tests.
   base::TimeDelta GetNudgeDelayTimeDelta(const syncer::ModelType& model_type);
 
-  // Set the internal scheduler for testing purposes.
-  // TODO(sync): Use dependency injection instead. crbug.com/133061
-  void SetSyncSchedulerForTest(
-      scoped_ptr<syncer::SyncScheduler> scheduler);
-
   base::ThreadChecker thread_checker_;
 
   // An opaque pointer to the nested private class.
@@ -570,6 +553,10 @@ class SyncManager {
 
 bool InitialSyncEndedForTypes(syncer::ModelTypeSet types, UserShare* share);
 
+syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
+    syncer::ModelTypeSet types,
+    syncer::UserShare* share);
+
 const char* ConnectionStatusToString(ConnectionStatus status);
 
 // Returns the string representation of a PassphraseRequiredReason value.
diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc
index e8ba9dd..c8a293e 100644
--- a/sync/internal_api/sync_manager.cc
+++ b/sync/internal_api/sync_manager.cc
@@ -197,11 +197,6 @@ class SyncManager::SyncInternal
   // went wrong.
   bool SignIn(const SyncCredentials& credentials);
 
-  // Purge from the directory those types with non-empty progress markers
-  // but without initial synced ended set.
-  // Returns false if an error occurred, true otherwise.
-  bool PurgePartiallySyncedTypes();
-
   // Update tokens that we're using in Sync. Email must stay the same.
   void UpdateCredentials(const SyncCredentials& credentials);
 
@@ -362,21 +357,6 @@ class SyncManager::SyncInternal
   // Called only by our NetworkChangeNotifier.
   virtual void OnIPAddressChanged() OVERRIDE;
 
-  ModelTypeSet GetTypesWithEmptyProgressMarkerToken(ModelTypeSet types) {
-    DCHECK(initialized_);
-    syncer::ModelTypeSet result;
-    for (syncer::ModelTypeSet::Iterator i = types.First();
-         i.Good(); i.Inc()) {
-      sync_pb::DataTypeProgressMarker marker;
-      directory()->GetDownloadProgress(i.Get(), &marker);
-
-      if (marker.token().empty())
-        result.Put(i.Get());
-
-    }
-    return result;
-  }
-
   syncer::ModelTypeSet InitialSyncEndedTypes() {
     DCHECK(initialized_);
     return directory()->initial_sync_ended_types();
@@ -396,8 +376,6 @@ class SyncManager::SyncInternal
       const std::string& name, const JsArgList& args,
       const WeakHandle<JsReplyHandler>& reply_handler) OVERRIDE;
 
-  void SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler);
-
  private:
   struct NotificationInfo {
     int total_count;
@@ -771,15 +749,6 @@ syncer::ModelTypeSet SyncManager::InitialSyncEndedTypes() {
   return data_->InitialSyncEndedTypes();
 }
 
-syncer::ModelTypeSet SyncManager::GetTypesWithEmptyProgressMarkerToken(
-    syncer::ModelTypeSet types) {
-  return data_->GetTypesWithEmptyProgressMarkerToken(types);
-}
-
-bool SyncManager::PurgePartiallySyncedTypes() {
-  return data_->PurgePartiallySyncedTypes();
-}
-
 void SyncManager::StartSyncingNormally(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -824,39 +793,40 @@ bool SyncManager::IsUsingExplicitPassphrase() {
   return data_ && data_->IsUsingExplicitPassphrase();
 }
 
-void SyncManager::ConfigureSyncer(
-    ConfigureReason reason,
-    const syncer::ModelTypeSet& types_to_config,
-    const syncer::ModelSafeRoutingInfo& new_routing_info,
-    const base::Closure& ready_task,
-    const base::Closure& retry_task) {
+void SyncManager::RequestCleanupDisabledTypes(
+    const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!ready_task.is_null());
-  DCHECK(!retry_task.is_null());
-
-  // TODO(zea): set this based on whether cryptographer has keystore
-  // encryption key or not (requires opening a transaction). crbug.com/129665.
-  ConfigurationParams::KeystoreKeyStatus keystore_key_status =
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY;
-
-  ConfigurationParams params(GetSourceFromReason(reason),
-                             types_to_config,
-                             new_routing_info,
-                             keystore_key_status,
-                             ready_task);
+  if (data_->scheduler()) {
+    data_->session_context()->set_routing_info(routing_info);
+    data_->scheduler()->CleanupDisabledTypes();
+  }
+}
 
+void SyncManager::RequestConfig(
+    const syncer::ModelSafeRoutingInfo& routing_info,
+    const ModelTypeSet& types, ConfigureReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!data_->scheduler()) {
     LOG(INFO)
-        << "SyncManager::ConfigureSyncer: could not configure because "
-        << "scheduler is null";
-    params.ready_task.Run();
+        << "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));
+}
 
-  data_->scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
-  if (!data_->scheduler()->ScheduleConfiguration(params))
-    retry_task.Run();
-
+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(
+      syncer::SyncScheduler::CONFIGURATION_MODE, callback);
 }
 
 bool SyncManager::SyncInternal::Init(
@@ -953,29 +923,16 @@ bool SyncManager::SyncInternal::Init(
     scheduler_.reset(new SyncScheduler(name_, session_context(), new Syncer()));
   }
 
-  bool success = SignIn(credentials);
+  bool signed_in = SignIn(credentials);
 
-  if (success) {
+  if (signed_in) {
     if (scheduler()) {
-      scheduler()->Start(syncer::SyncScheduler::CONFIGURATION_MODE);
+      scheduler()->Start(
+          syncer::SyncScheduler::CONFIGURATION_MODE, base::Closure());
     }
 
     initialized_ = true;
 
-    // Unapplied datatypes (those that do not have initial sync ended set) get
-    // re-downloaded during any configuration. But, it's possible for a datatype
-    // to have a progress marker but not have initial sync ended yet, making
-    // it a candidate for migration. This is a problem, as the DataTypeManager
-    // does not support a migration while it's already in the middle of a
-    // configuration. As a result, any partially synced datatype can stall the
-    // DTM, waiting for the configuration to complete, which it never will due
-    // to the migration error. In addition, a partially synced nigori will
-    // trigger the migration logic before the backend is initialized, resulting
-    // in crashes. We therefore detect and purge any partially synced types as
-    // part of initialization.
-    if (!PurgePartiallySyncedTypes())
-      success = false;
-
     // 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
@@ -993,14 +950,14 @@ bool SyncManager::SyncInternal::Init(
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        success));
+                        signed_in));
 
-  if (!success && testing_mode_ == NON_TEST)
+  if (!signed_in && testing_mode_ == NON_TEST)
     return false;
 
   sync_notifier_->AddObserver(this);
 
-  return success;
+  return signed_in;
 }
 
 void SyncManager::SyncInternal::UpdateCryptographerAndNigori(
@@ -1145,13 +1102,9 @@ void SyncManager::SyncInternal::NotifyCryptographerState(
 void SyncManager::SyncInternal::StartSyncingNormally(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   // Start the sync scheduler.
-  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
-    // appropriately set and that it's only modified when switching to normal
-    // mode.
+  if (scheduler()) { // NULL during certain unittests.
     session_context()->set_routing_info(routing_info);
-    scheduler()->Start(SyncScheduler::NORMAL_MODE);
+    scheduler()->Start(SyncScheduler::NORMAL_MODE, base::Closure());
   }
 }
 
@@ -1205,20 +1158,6 @@ bool SyncManager::SyncInternal::SignIn(const SyncCredentials& credentials) {
   return true;
 }
 
-bool SyncManager::SyncInternal::PurgePartiallySyncedTypes() {
-  syncer::ModelTypeSet partially_synced_types =
-      syncer::ModelTypeSet::All();
-  partially_synced_types.RemoveAll(InitialSyncEndedTypes());
-  partially_synced_types.RemoveAll(GetTypesWithEmptyProgressMarkerToken(
-      syncer::ModelTypeSet::All()));
-
-  UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
-                       partially_synced_types.Size());
-  if (partially_synced_types.Empty())
-    return true;
-  return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
-}
-
 void SyncManager::SyncInternal::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -2401,11 +2340,6 @@ void SyncManager::SyncInternal::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void SyncManager::SyncInternal::SetSyncSchedulerForTest(
-    scoped_ptr<SyncScheduler> sync_scheduler) {
-  scheduler_ = sync_scheduler.Pass();
-}
-
 SyncStatus SyncManager::GetDetailedStatus() const {
   return data_->GetStatus();
 }
@@ -2436,10 +2370,6 @@ TimeDelta SyncManager::GetNudgeDelayTimeDelta(
   return data_->GetNudgeDelayTimeDelta(model_type);
 }
 
-void SyncManager::SetSyncSchedulerForTest(scoped_ptr<SyncScheduler> scheduler) {
-  data_->SetSyncSchedulerForTest(scheduler.Pass());
-}
-
 syncer::ModelTypeSet SyncManager::GetEncryptedDataTypesForTest() const {
   ReadTransaction trans(FROM_HERE, GetUserShare());
   return GetEncryptedTypes(&trans);
@@ -2529,4 +2459,20 @@ bool InitialSyncEndedForTypes(syncer::ModelTypeSet types,
   return true;
 }
 
+syncer::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
+    syncer::ModelTypeSet types,
+    syncer::UserShare* share) {
+  syncer::ModelTypeSet result;
+  for (syncer::ModelTypeSet::Iterator i = types.First();
+       i.Good(); i.Inc()) {
+    sync_pb::DataTypeProgressMarker marker;
+    share->directory->GetDownloadProgress(i.Get(), &marker);
+
+    if (marker.token().empty())
+      result.Put(i.Get());
+
+  }
+  return result;
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc
index b22b585..6fff8c5 100644
--- a/sync/internal_api/syncapi_unittest.cc
+++ b/sync/internal_api/syncapi_unittest.cc
@@ -24,7 +24,6 @@
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base/model_type_test_util.h"
-#include "sync/engine/sync_scheduler.h"
 #include "sync/internal_api/public/change_record.h"
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
@@ -58,7 +57,6 @@
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/syncable/write_transaction.h"
-#include "sync/test/callback_counter.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/fake_extensions_activity_monitor.h"
 #include "sync/util/cryptographer.h"
@@ -72,10 +70,8 @@ using base::ExpectDictStringValue;
 using testing::_;
 using testing::AnyNumber;
 using testing::AtLeast;
-using testing::DoAll;
 using testing::InSequence;
 using testing::Invoke;
-using testing::Return;
 using testing::SaveArg;
 using testing::StrictMock;
 
@@ -910,10 +906,6 @@ class SyncManagerTest : public testing::Test,
     return true;
   }
 
-  void SetScheduler(scoped_ptr<SyncScheduler> scheduler) {
-    sync_manager_.SetSyncSchedulerForTest(scheduler.Pass());
-  }
-
  private:
   // Needed by |sync_manager_|.
   MessageLoop message_loop_;
@@ -2493,120 +2485,4 @@ TEST_F(SyncManagerTest, SetPreviouslyEncryptedSpecifics) {
   }
 }
 
-class MockSyncScheduler : public SyncScheduler {
- public:
-  MockSyncScheduler() : SyncScheduler("name", NULL, NULL) {}
-  virtual ~MockSyncScheduler() {}
-
-  MOCK_METHOD1(Start, void(SyncScheduler::Mode));
-  MOCK_METHOD1(ScheduleConfiguration, bool(const ConfigurationParams&));
-};
-
-// Test that the configuration params are properly created and sent to
-// ScheduleConfigure. No callback should be invoked.
-TEST_F(SyncManagerTest, BasicConfiguration) {
-  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
-  syncer::ModelTypeSet types_to_download(syncer::BOOKMARKS,
-                                         syncer::PREFERENCES);
-  syncer::ModelSafeRoutingInfo new_routing_info;
-  GetModelSafeRoutingInfo(&new_routing_info);
-
-  scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler());
-  ConfigurationParams params;
-  EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler, ScheduleConfiguration(_)).
-      WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
-  SetScheduler(scheduler.PassAs<SyncScheduler>());
-
-  CallbackCounter ready_task_counter, retry_task_counter;
-  sync_manager_.ConfigureSyncer(
-      reason,
-      types_to_download,
-      new_routing_info,
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&ready_task_counter)),
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&retry_task_counter)));
-  EXPECT_EQ(0, ready_task_counter.times_called());
-  EXPECT_EQ(0, retry_task_counter.times_called());
-  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
-            params.source);
-  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
-  EXPECT_EQ(new_routing_info, params.routing_info);
-}
-
-// Test that the retry callback is invoked on configuration failure.
-TEST_F(SyncManagerTest, ConfigurationRetry) {
-  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
-  syncer::ModelTypeSet types_to_download(syncer::BOOKMARKS,
-                                         syncer::PREFERENCES);
-  syncer::ModelSafeRoutingInfo new_routing_info;
-  GetModelSafeRoutingInfo(&new_routing_info);
-
-  scoped_ptr<MockSyncScheduler> scheduler(new MockSyncScheduler());
-  ConfigurationParams params;
-  EXPECT_CALL(*scheduler, Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler, ScheduleConfiguration(_)).
-      WillOnce(DoAll(SaveArg<0>(&params), Return(false)));
-  SetScheduler(scheduler.PassAs<SyncScheduler>());
-
-  CallbackCounter ready_task_counter, retry_task_counter;
-  sync_manager_.ConfigureSyncer(
-      reason,
-      types_to_download,
-      new_routing_info,
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&ready_task_counter)),
-      base::Bind(&CallbackCounter::Callback,
-                 base::Unretained(&retry_task_counter)));
-  EXPECT_EQ(0, ready_task_counter.times_called());
-  EXPECT_EQ(1, retry_task_counter.times_called());
-  EXPECT_EQ(sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
-            params.source);
-  EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
-  EXPECT_EQ(new_routing_info, params.routing_info);
-}
-
-// Test that PurgePartiallySyncedTypes purges only those types that don't
-// have empty progress marker and don't have initial sync ended set.
-TEST_F(SyncManagerTest, PurgePartiallySyncedTypes) {
-  UserShare* share = sync_manager_.GetUserShare();
-
-  // Set Nigori and Bookmarks to be partial types.
-  sync_pb::DataTypeProgressMarker nigori_marker;
-  nigori_marker.set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(NIGORI));
-  nigori_marker.set_token("token");
-  sync_pb::DataTypeProgressMarker bookmark_marker;
-  bookmark_marker.set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(BOOKMARKS));
-  bookmark_marker.set_token("token");
-  share->directory->SetDownloadProgress(NIGORI, nigori_marker);
-  share->directory->SetDownloadProgress(BOOKMARKS, bookmark_marker);
-
-  // Set Preferences to be a full type.
-  sync_pb::DataTypeProgressMarker pref_marker;
-  pref_marker.set_data_type_id(
-      GetSpecificsFieldNumberFromModelType(PREFERENCES));
-  pref_marker.set_token("token");
-  share->directory->SetDownloadProgress(PREFERENCES, pref_marker);
-  share->directory->set_initial_sync_ended_for_type(PREFERENCES, true);
-
-  ModelTypeSet partial_types =
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
-  EXPECT_FALSE(partial_types.Has(NIGORI));
-  EXPECT_FALSE(partial_types.Has(BOOKMARKS));
-  EXPECT_FALSE(partial_types.Has(PREFERENCES));
-
-  EXPECT_TRUE(sync_manager_.PurgePartiallySyncedTypes());
-
-  // Ensure only bookmarks and nigori lost their progress marker. Preferences
-  // should still have it.
-  partial_types =
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All());
-  EXPECT_TRUE(partial_types.Has(NIGORI));
-  EXPECT_TRUE(partial_types.Has(BOOKMARKS));
-  EXPECT_FALSE(partial_types.Has(PREFERENCES));
-}
-
-}  // namespace
+}  // namespace syncer