Revert 142517 - [Sync] Refactor sync configuration logic.

We remove all the pending download/configure state in SBH, in addition
to the split transaction nature of configurations themselves. This allows
us to have a single SyncScheduler::ScheduleConfiguration command that is
both synchronous (assuming it doesn't fail) and can handle
 CleanupDisabledTypes and GetKey commands.

This also now keys which datatypes need downloading by checking the
initial sync ended bits directly. This allows us to recover from a new
sync db gracefully.

BUG=129665,133061,129825
TEST=unit/integration tests

Review URL: https://chromiumcodereview.appspot.com/10483015

TBR=zea@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10583002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142814 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 181 insertions, 317 deletions

diff --git a/sync/engine/sync_scheduler.cc b/sync/engine/sync_scheduler.cc
index e745d19..f798d67 100644
--- a/sync/engine/sync_scheduler.cc
+++ b/sync/engine/sync_scheduler.cc
@@ -68,24 +68,6 @@ bool IsActionableError(
 }
 }  // namespace
 
-ConfigurationParams::ConfigurationParams()
-    : source(GetUpdatesCallerInfo::UNKNOWN),
-      keystore_key_status(KEYSTORE_KEY_UNNECESSARY) {}
-ConfigurationParams::ConfigurationParams(
-    const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source,
-    const syncable::ModelTypeSet& types_to_download,
-    const browser_sync::ModelSafeRoutingInfo& routing_info,
-    KeystoreKeyStatus keystore_key_status,
-    const base::Closure& ready_task)
-    : source(source),
-      types_to_download(types_to_download),
-      routing_info(routing_info),
-      keystore_key_status(keystore_key_status),
-      ready_task(ready_task) {
-  DCHECK(!ready_task.is_null());
-}
-ConfigurationParams::~ConfigurationParams() {}
-
 SyncScheduler::DelayProvider::DelayProvider() {}
 SyncScheduler::DelayProvider::~DelayProvider() {}
 
@@ -117,16 +99,12 @@ SyncScheduler::SyncSessionJob::~SyncSessionJob() {}
 
 SyncScheduler::SyncSessionJob::SyncSessionJob(SyncSessionJobPurpose purpose,
     base::TimeTicks start,
-    linked_ptr<sessions::SyncSession> session,
-    bool is_canary_job,
-    ConfigurationParams config_params,
-    const tracked_objects::Location& from_here)
-    : purpose(purpose),
-      scheduled_start(start),
-      session(session),
-      is_canary_job(is_canary_job),
-      config_params(config_params),
-      from_here(from_here) {
+    linked_ptr<sessions::SyncSession> session, bool is_canary_job,
+    const tracked_objects::Location& from_here) : purpose(purpose),
+        scheduled_start(start),
+        session(session),
+        is_canary_job(is_canary_job),
+        from_here(from_here) {
 }
 
 const char* SyncScheduler::SyncSessionJob::GetPurposeString(
@@ -269,7 +247,7 @@ void SyncScheduler::UpdateServerConnectionManagerStatus(
   connection_code_ = code;
 }
 
-void SyncScheduler::Start(Mode mode) {
+void SyncScheduler::Start(Mode mode, const base::Closure& callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   std::string thread_name = MessageLoop::current()->thread_name();
   if (thread_name.empty())
@@ -286,6 +264,8 @@ void SyncScheduler::Start(Mode mode) {
   Mode old_mode = mode_;
   mode_ = mode;
   AdjustPolling(NULL);  // Will kick start poll timer if needed.
+  if (!callback.is_null())
+    callback.Run();
 
   if (old_mode != mode_) {
     // We just changed our mode. See if there are any pending jobs that we could
@@ -304,112 +284,6 @@ void SyncScheduler::SendInitialSnapshot() {
   session_context_->NotifyListeners(event);
 }
 
-namespace {
-
-// Helper to extract the routing info and workers corresponding to types in
-// |types| from |current_routes| and |current_workers|.
-void BuildModelSafeParams(
-    const ModelTypeSet& types_to_download,
-    const ModelSafeRoutingInfo& current_routes,
-    const std::vector<ModelSafeWorker*>& current_workers,
-    ModelSafeRoutingInfo* result_routes,
-    std::vector<ModelSafeWorker*>* result_workers) {
-  std::set<ModelSafeGroup> active_groups;
-  active_groups.insert(GROUP_PASSIVE);
-  for (ModelTypeSet::Iterator iter = types_to_download.First(); iter.Good();
-       iter.Inc()) {
-    syncable::ModelType type = iter.Get();
-    ModelSafeRoutingInfo::const_iterator route = current_routes.find(type);
-    DCHECK(route != current_routes.end());
-    ModelSafeGroup group = route->second;
-    (*result_routes)[type] = group;
-    active_groups.insert(group);
-  }
-
-  for(std::vector<ModelSafeWorker*>::const_iterator iter =
-          current_workers.begin(); iter != current_workers.end(); ++iter) {
-    if (active_groups.count((*iter)->GetModelSafeGroup()) > 0)
-      result_workers->push_back(*iter);
-  }
-}
-
-}  // namespace.
-
-bool SyncScheduler::ScheduleConfiguration(const ConfigurationParams& params) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  DCHECK(IsConfigRelatedUpdateSourceValue(params.source));
-  DCHECK_EQ(CONFIGURATION_MODE, mode_);
-  DCHECK(!params.ready_task.is_null());
-  SDVLOG(2) << "Reconfiguring syncer.";
-
-  // Only one configuration is allowed at a time. Verify we're not waiting
-  // for a pending configure job.
-  DCHECK(!wait_interval_.get() || !wait_interval_->pending_configure_job.get());
-
-  // TODO(sync): now that ModelChanging commands only use those workers within
-  // the routing info, we don't really need |restricted_workers|. Remove it.
-  // crbug.com/133030
-  browser_sync::ModelSafeRoutingInfo restricted_routes;
-  std::vector<ModelSafeWorker*> restricted_workers;
-  BuildModelSafeParams(params.types_to_download,
-                       params.routing_info,
-                       session_context_->workers(),
-                       &restricted_routes,
-                       &restricted_workers);
-  session_context_->set_routing_info(params.routing_info);
-
-  // We rely on this not failing, so don't need to worry about checking for
-  // success. In addition, this will be removed as part of crbug.com/131433.
-  SyncSessionJob cleanup_job(
-      SyncSessionJob::CLEANUP_DISABLED_TYPES,
-      TimeTicks::Now(),
-      make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
-      false,
-      ConfigurationParams(),
-      FROM_HERE);
-  DoSyncSessionJob(cleanup_job);
-
-  if (params.keystore_key_status == ConfigurationParams::KEYSTORE_KEY_NEEDED) {
-    // TODO(zea): implement in such a way that we can handle failures and the
-    // subsequent retrys the scheduler might perform. See crbug.com/129665.
-    NOTIMPLEMENTED();
-  }
-
-  // Only reconfigure if we have types to download.
-  if (!params.types_to_download.Empty()) {
-    DCHECK(!restricted_routes.empty());
-    linked_ptr<SyncSession> session(new SyncSession(
-        session_context_,
-        this,
-        SyncSourceInfo(params.source,
-                       syncable::ModelTypePayloadMapFromRoutingInfo(
-                           restricted_routes,
-                           std::string())),
-        restricted_routes,
-        restricted_workers));
-    SyncSessionJob job(SyncSessionJob::CONFIGURATION,
-                       TimeTicks::Now(),
-                       session,
-                       false,
-                       params,
-                       FROM_HERE);
-    DoSyncSessionJob(job);
-
-    // If we failed, the job would have been saved as the pending configure
-    // job and a wait interval would have been set.
-    if (!session->Succeeded()) {
-      DCHECK(wait_interval_.get() &&
-             wait_interval_->pending_configure_job.get());
-      return false;
-    }
-  } else {
-    SDVLOG(2) << "No change in routing info, calling ready task directly.";
-    params.ready_task.Run();
-  }
-
-  return true;
-}
-
 SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ -508,8 +382,7 @@ void SyncScheduler::InitOrCoalescePendingJob(const SyncSessionJob& job) {
         s->delegate(), s->source(), s->routing_info(), s->workers()));
 
     SyncSessionJob new_job(SyncSessionJob::NUDGE, job.scheduled_start,
-        make_linked_ptr(session.release()), false,
-        ConfigurationParams(), job.from_here);
+        make_linked_ptr(session.release()), false, job.from_here);
     pending_nudge_.reset(new SyncSessionJob(new_job));
 
     return;
@@ -555,17 +428,11 @@ void SyncScheduler::SaveJob(const SyncSessionJob& job) {
     DCHECK(wait_interval_.get());
     DCHECK(mode_ == CONFIGURATION_MODE);
 
-    // Config params should always get set.
-    DCHECK(!job.config_params.ready_task.is_null());
     SyncSession* old = job.session.get();
     SyncSession* s(new SyncSession(session_context_, this, old->source(),
                                    old->routing_info(), old->workers()));
-    SyncSessionJob new_job(job.purpose,
-                           TimeTicks::Now(),
-                           make_linked_ptr(s),
-                           false,
-                           job.config_params,
-                           job.from_here);
+    SyncSessionJob new_job(job.purpose, TimeTicks::Now(),
+                           make_linked_ptr(s), false, job.from_here);
     wait_interval_->pending_configure_job.reset(new SyncSessionJob(new_job));
   } // drop the rest.
   // TODO(sync): Is it okay to drop the rest?  It's weird that
@@ -584,16 +451,23 @@ struct ModelSafeWorkerGroupIs {
 
 void SyncScheduler::ClearUserData() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA,
-                     TimeTicks::Now(),
+  SyncSessionJob job(SyncSessionJob::CLEAR_USER_DATA, TimeTicks::Now(),
                      make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
                      false,
-                     ConfigurationParams(),
                      FROM_HERE);
 
   DoSyncSessionJob(job);
 }
 
+void SyncScheduler::CleanupDisabledTypes() {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  SyncSessionJob job(SyncSessionJob::CLEANUP_DISABLED_TYPES, TimeTicks::Now(),
+                     make_linked_ptr(CreateSyncSession(SyncSourceInfo())),
+                     false,
+                     FROM_HERE);
+  DoSyncSessionJob(job);
+}
+
 void SyncScheduler::ScheduleNudgeAsync(
     const TimeDelta& delay,
     NudgeSource source, ModelTypeSet types,
@@ -651,7 +525,7 @@ void SyncScheduler::ScheduleNudgeImpl(
   SyncSession* session(CreateSyncSession(info));
   SyncSessionJob job(SyncSessionJob::NUDGE, TimeTicks::Now() + delay,
                      make_linked_ptr(session), is_canary_job,
-                     ConfigurationParams(), nudge_location);
+                     nudge_location);
 
   session = NULL;
   if (!ShouldRunJob(job))
@@ -682,6 +556,77 @@ void SyncScheduler::ScheduleNudgeImpl(
   ScheduleSyncSessionJob(job);
 }
 
+// Helper to extract the routing info and workers corresponding to types in
+// |types| from |current_routes| and |current_workers|.
+void GetModelSafeParamsForTypes(ModelTypeSet types,
+    const ModelSafeRoutingInfo& current_routes,
+    const std::vector<ModelSafeWorker*>& current_workers,
+    ModelSafeRoutingInfo* result_routes,
+    std::vector<ModelSafeWorker*>* result_workers) {
+  bool passive_group_added = false;
+
+  typedef std::vector<ModelSafeWorker*>::const_iterator iter;
+  for (ModelTypeSet::Iterator it = types.First();
+       it.Good(); it.Inc()) {
+    const syncable::ModelType t = it.Get();
+    ModelSafeRoutingInfo::const_iterator route = current_routes.find(t);
+    DCHECK(route != current_routes.end());
+    ModelSafeGroup group = route->second;
+
+    (*result_routes)[t] = group;
+    iter w_tmp_it = std::find_if(current_workers.begin(), current_workers.end(),
+                                 ModelSafeWorkerGroupIs(group));
+    if (w_tmp_it != current_workers.end()) {
+      iter result_workers_it = std::find_if(
+          result_workers->begin(), result_workers->end(),
+          ModelSafeWorkerGroupIs(group));
+      if (result_workers_it == result_workers->end())
+        result_workers->push_back(*w_tmp_it);
+
+      if (group == GROUP_PASSIVE)
+        passive_group_added = true;
+    } else {
+        NOTREACHED();
+    }
+  }
+
+  // Always add group passive.
+  if (passive_group_added == false) {
+    iter it = std::find_if(current_workers.begin(), current_workers.end(),
+                           ModelSafeWorkerGroupIs(GROUP_PASSIVE));
+    if (it != current_workers.end())
+      result_workers->push_back(*it);
+    else
+      NOTREACHED();
+  }
+}
+
+void SyncScheduler::ScheduleConfiguration(
+    ModelTypeSet types,
+    GetUpdatesCallerInfo::GetUpdatesSource source) {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  DCHECK(IsConfigRelatedUpdateSourceValue(source));
+  SDVLOG(2) << "Scheduling a config";
+
+  ModelSafeRoutingInfo routes;
+  std::vector<ModelSafeWorker*> workers;
+  GetModelSafeParamsForTypes(types,
+                             session_context_->routing_info(),
+                             session_context_->workers(),
+                             &routes, &workers);
+
+  SyncSession* session = new SyncSession(session_context_, this,
+      SyncSourceInfo(source,
+                     syncable::ModelTypePayloadMapFromRoutingInfo(
+                         routes, std::string())),
+      routes, workers);
+  SyncSessionJob job(SyncSessionJob::CONFIGURATION, TimeTicks::Now(),
+                     make_linked_ptr(session),
+                     false,
+                     FROM_HERE);
+  DoSyncSessionJob(job);
+}
+
 const char* SyncScheduler::GetModeString(SyncScheduler::Mode mode) {
   switch (mode) {
     ENUM_CASE(CONFIGURATION_MODE);
@@ -884,11 +829,6 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
     // here; see DCHECKs in SaveJob().  (See http://crbug.com/90868.)
     SaveJob(job);
     return;  // Nothing to do.
-  } else if (job.session->Succeeded() &&
-             !job.config_params.ready_task.is_null()) {
-    // If this was a configuration job with a ready task, invoke it now that
-    // we finished successfully.
-    job.config_params.ready_task.Run();
   }
 
   SDVLOG(2) << "Updating the next polling time after SyncMain";
@@ -991,15 +931,11 @@ void SyncScheduler::HandleContinuationError(
   wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF,
                                         length));
   if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
-    SDVLOG(2) << "Configuration did not succeed, scheduling retry.";
-    // Config params should always get set.
-    DCHECK(!old_job.config_params.ready_task.is_null());
     SyncSession* old = old_job.session.get();
     SyncSession* s(new SyncSession(session_context_, this,
         old->source(), old->routing_info(), old->workers()));
     SyncSessionJob job(old_job.purpose, TimeTicks::Now() + length,
-                       make_linked_ptr(s), false, old_job.config_params,
-                       FROM_HERE);
+                        make_linked_ptr(s), false, FROM_HERE);
     wait_interval_->pending_configure_job.reset(new SyncSessionJob(job));
   } else {
     // We are not in configuration mode. So wait_interval's pending job
@@ -1121,7 +1057,6 @@ void SyncScheduler::PollTimerCallback() {
   SyncSessionJob job(SyncSessionJob::POLL, TimeTicks::Now(),
                      make_linked_ptr(s),
                      false,
-                     ConfigurationParams(),
                      FROM_HERE);
 
   ScheduleSyncSessionJob(job);
diff --git a/sync/engine/sync_scheduler.h b/sync/engine/sync_scheduler.h
index 159d759..f6a54cc 100644
--- a/sync/engine/sync_scheduler.h
+++ b/sync/engine/sync_scheduler.h
@@ -37,32 +37,6 @@ namespace browser_sync {
 
 struct ServerConnectionEvent;
 
-struct ConfigurationParams {
-  enum KeystoreKeyStatus {
-    KEYSTORE_KEY_UNNECESSARY,
-    KEYSTORE_KEY_NEEDED
-  };
-  ConfigurationParams();
-  ConfigurationParams(
-      const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource& source,
-      const syncable::ModelTypeSet& types_to_download,
-      const browser_sync::ModelSafeRoutingInfo& routing_info,
-      KeystoreKeyStatus keystore_key_status,
-      const base::Closure& ready_task);
-  ~ConfigurationParams();
-
-  // Source for the configuration.
-  sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source;
-  // The types that should be downloaded.
-  syncable::ModelTypeSet types_to_download;
-  // The new routing info (superset of types to be downloaded).
-  ModelSafeRoutingInfo routing_info;
-  // Whether we need to perform a GetKey command.
-  KeystoreKeyStatus keystore_key_status;
-  // Callback to invoke on configuration completion.
-  base::Closure ready_task;
-};
-
 class SyncScheduler : public sessions::SyncSession::Delegate {
  public:
   enum Mode {
@@ -89,15 +63,10 @@ class SyncScheduler : public sessions::SyncSession::Delegate {
 
   // Start the scheduler with the given mode.  If the scheduler is
   // already started, switch to the given mode, although some
-  // scheduled tasks from the old mode may still run.
-  virtual void Start(Mode mode);
-
-  // Schedules the configuration task specified by |params|. Returns true if
-  // the configuration task executed immediately, false if it had to be
-  // scheduled for a later attempt. |params.ready_task| is invoked whenever the
-  // configuration task executes.
-  // Note: must already be in CONFIGURATION mode.
-  virtual bool ScheduleConfiguration(const ConfigurationParams& params);
+  // scheduled tasks from the old mode may still run.  If non-NULL,
+  // |callback| will be invoked when the mode has been changed to
+  // |mode|.  Takes ownership of |callback|.
+  void Start(Mode mode, const base::Closure& callback);
 
   // Request that any running syncer task stop as soon as possible and
   // cancel all scheduled tasks. This function can be called from any thread,
@@ -117,6 +86,14 @@ class SyncScheduler : public sessions::SyncSession::Delegate {
       const syncable::ModelTypePayloadMap& types_with_payloads,
       const tracked_objects::Location& nudge_location);
 
+  // Schedule a configuration cycle. May execute immediately or at a later time
+  // (depending on backoff/throttle state).
+  // Note: The source argument of this function must come from the subset of
+  // GetUpdatesCallerInfo values related to configurations.
+  void ScheduleConfiguration(
+      syncable::ModelTypeSet types,
+      sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source);
+
   // TODO(tim): remove this. crbug.com/131336
   void ClearUserData();
 
@@ -187,7 +164,6 @@ class SyncScheduler : public sessions::SyncSession::Delegate {
     SyncSessionJob();
     SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start,
         linked_ptr<sessions::SyncSession> session, bool is_canary_job,
-        ConfigurationParams config_params,
         const tracked_objects::Location& nudge_location);
     ~SyncSessionJob();
     static const char* GetPurposeString(SyncSessionJobPurpose purpose);
@@ -196,7 +172,6 @@ class SyncScheduler : public sessions::SyncSession::Delegate {
     base::TimeTicks scheduled_start;
     linked_ptr<sessions::SyncSession> session;
     bool is_canary_job;
-    ConfigurationParams config_params;
 
     // This is the location the job came from.  Used for debugging.
     // In case of multiple nudges getting coalesced this stores the
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 7aa30ab..a6d8059 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -12,7 +12,6 @@
 #include "sync/engine/syncer.h"
 #include "sync/engine/throttled_data_type_tracker.h"
 #include "sync/sessions/test_util.h"
-#include "sync/test/callback_counter.h"
 #include "sync/test/engine/fake_model_worker.h"
 #include "sync/test/engine/mock_connection_manager.h"
 #include "sync/test/engine/test_directory_setter_upper.h"
@@ -28,7 +27,6 @@ using testing::DoAll;
 using testing::Eq;
 using testing::Invoke;
 using testing::Mock;
-using testing::Not;
 using testing::Return;
 using testing::WithArg;
 
@@ -71,14 +69,6 @@ void PumpLoop() {
   RunLoop();
 }
 
-ModelSafeRoutingInfo TypesToRoutingInfo(const ModelTypeSet& types) {
-  ModelSafeRoutingInfo routes;
-  for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) {
-    routes[iter.Get()] = GROUP_PASSIVE;
-  }
-  return routes;
-}
-
 // Convenient to use in tests wishing to analyze SyncShare calls over time.
 static const size_t kMinNumSamples = 5;
 class SyncSchedulerTest : public testing::Test {
@@ -162,7 +152,7 @@ class SyncSchedulerTest : public testing::Test {
   }
 
   void StartSyncScheduler(SyncScheduler::Mode mode) {
-    scheduler()->Start(mode);
+    scheduler()->Start(mode, base::Closure());
   }
 
   // This stops the scheduler synchronously.
@@ -301,23 +291,14 @@ TEST_F(SyncSchedulerTest, Config) {
   SyncShareRecords records;
   const ModelTypeSet model_types(syncable::BOOKMARKS);
 
-  EXPECT_CALL(*syncer(),
-              SyncShare(_,_,_))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      model_types,
-      TypesToRoutingInfo(model_types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(1, counter.times_called());
+  scheduler()->ScheduleConfiguration(
+      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
 
   ASSERT_EQ(1U, records.snapshots.size());
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types,
@@ -334,9 +315,7 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   SyncShareRecords records;
   const ModelTypeSet model_types(syncable::BOOKMARKS);
 
-  EXPECT_CALL(*syncer(),
-              SyncShare(_,_,_))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
                       WithArg<0>(RecordSyncShare(&records))))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
@@ -345,27 +324,61 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ASSERT_EQ(0U, records.snapshots.size());
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      model_types,
-      TypesToRoutingInfo(model_types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+  scheduler()->ScheduleConfiguration(
+      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
 
   ASSERT_EQ(1U, records.snapshots.size());
   RunLoop();
 
   ASSERT_EQ(2U, records.snapshots.size());
-  ASSERT_EQ(1, counter.times_called());
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types,
       records.snapshots[1].source().types));
   EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
             records.snapshots[1].source().updates_source);
 }
 
+// Issue 2 config commands. Second one right after the first has failed
+// and make sure LATEST is executed.
+TEST_F(SyncSchedulerTest, MultipleConfigWithBackingOff) {
+  const ModelTypeSet
+      model_types1(syncable::BOOKMARKS),
+      model_types2(syncable::AUTOFILL);
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(TimeDelta::FromMilliseconds(30)));
+  SyncShareRecords records;
+
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+                      WithArg<0>(RecordSyncShare(&records))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
+                      WithArg<0>(RecordSyncShare(&records))))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+                      WithArg<0>(RecordSyncShare(&records))));
+
+  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
+
+  ASSERT_EQ(0U, records.snapshots.size());
+  scheduler()->ScheduleConfiguration(
+      model_types1, GetUpdatesCallerInfo::RECONFIGURATION);
+
+  ASSERT_EQ(1U, records.snapshots.size());
+  scheduler()->ScheduleConfiguration(
+      model_types2, GetUpdatesCallerInfo::RECONFIGURATION);
+
+  // A canary job gets posted when we go into exponential backoff.
+  RunLoop();
+
+  ASSERT_EQ(2U, records.snapshots.size());
+  RunLoop();
+
+  ASSERT_EQ(3U, records.snapshots.size());
+  EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(model_types2,
+      records.snapshots[2].source().types));
+  EXPECT_EQ(GetUpdatesCallerInfo::RECONFIGURATION,
+            records.snapshots[2].source().updates_source);
+}
+
 // Issue a nudge when the config has failed. Make sure both the config and
 // nudge are executed.
 TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
@@ -388,28 +401,19 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
   ASSERT_EQ(0U, records.snapshots.size());
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      model_types,
-      TypesToRoutingInfo(model_types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
-  ASSERT_EQ(1U, records.snapshots.size());
+  scheduler()->ScheduleConfiguration(
+      model_types, GetUpdatesCallerInfo::RECONFIGURATION);
 
+  ASSERT_EQ(1U, records.snapshots.size());
   scheduler()->ScheduleNudgeAsync(
       zero(), NUDGE_SOURCE_LOCAL, model_types, FROM_HERE);
   RunLoop();
-  ASSERT_EQ(2U, records.snapshots.size());
-  ASSERT_EQ(0, counter.times_called());
 
+  ASSERT_EQ(2U, records.snapshots.size());
   RunLoop();
-  ASSERT_EQ(3U, records.snapshots.size());
-  ASSERT_EQ(1, counter.times_called());
 
   // Now change the mode so nudge can execute.
+  ASSERT_EQ(3U, records.snapshots.size());
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   ASSERT_EQ(4U, records.snapshots.size());
@@ -714,19 +718,13 @@ TEST_F(SyncSchedulerTest, HasMoreToSyncThenFails) {
   EXPECT_TRUE(RunAndGetBackoff());
 }
 
-// Test that no syncing occurs when throttled (although CleanupDisabledTypes
-// is allowed).
+// Test that no syncing occurs when throttled.
 TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   const ModelTypeSet types(syncable::BOOKMARKS);
   TimeDelta poll(TimeDelta::FromMilliseconds(5));
   TimeDelta throttle(TimeDelta::FromMinutes(10));
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
-
-  EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
-  EXPECT_CALL(*syncer(), SyncShare(_,Not(CLEANUP_DISABLED_TYPES),
-                                     Not(CLEANUP_DISABLED_TYPES)))
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
       .WillOnce(WithArg<0>(sessions::test_util::SimulateThrottled(throttle)))
       .WillRepeatedly(AddFailureAndQuitLoopNow());
 
@@ -738,15 +736,8 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      types,
-      TypesToRoutingInfo(types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+  scheduler()->ScheduleConfiguration(
+      types, GetUpdatesCallerInfo::RECONFIGURATION);
 }
 
 TEST_F(SyncSchedulerTest, ThrottlingExpires) {
@@ -779,7 +770,6 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
   SyncShareRecords records;
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
   EXPECT_CALL(*syncer(), SyncShare(_,_,_))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess))
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
                       WithArg<0>(RecordSyncShare(&records))));
 
@@ -793,15 +783,8 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
 
   const ModelTypeSet config_types(syncable::BOOKMARKS);
 
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      config_types,
-      TypesToRoutingInfo(config_types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(1, counter.times_called());
+  scheduler()->ScheduleConfiguration(
+      config_types, GetUpdatesCallerInfo::RECONFIGURATION);
 
   ASSERT_EQ(1U, records.snapshots.size());
   EXPECT_TRUE(CompareModelTypeSetToModelTypePayloadMap(config_types,
@@ -870,7 +853,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   scheduler()->OnReceivedLongPollIntervalUpdate(poll);
   UseMockDelayProvider();
 
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1)
       .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
                             RecordSyncShareMultiple(&r, 1U)));
   EXPECT_CALL(*delay(), GetDelay(_)).
@@ -912,15 +895,9 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      types,
-      TypesToRoutingInfo(types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_FALSE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(0, counter.times_called());
+  scheduler()->CleanupDisabledTypes();
+  scheduler()->ScheduleConfiguration(
+      types, GetUpdatesCallerInfo::RECONFIGURATION);
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
@@ -1089,49 +1066,28 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  // Configuration (always includes a cleanup disabled types).
-  EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  // Configuration.
   EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  syncable::ModelTypeSet model_types(syncable::BOOKMARKS);
-  CallbackCounter counter;
-  ConfigurationParams params(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      model_types,
-      TypesToRoutingInfo(model_types),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params));
-  ASSERT_EQ(1, counter.times_called());
-  // Runs directly so no need to pump the loop.
+  scheduler()->ScheduleConfiguration(
+      ModelTypeSet(), GetUpdatesCallerInfo::RECONFIGURATION);
+
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  // Cleanup disabled types. Because no types are being configured, we just
-  // perform the cleanup.
+  // Cleanup disabled types.
   EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)).
-      WillOnce(Invoke(sessions::test_util::SimulateSuccess));
-  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
+              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
+      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  scheduler()->CleanupDisabledTypes();
 
-  CallbackCounter counter2;
-  ConfigurationParams params2(
-      GetUpdatesCallerInfo::RECONFIGURATION,
-      ModelTypeSet(),
-      ModelSafeRoutingInfo(),
-      ConfigurationParams::KEYSTORE_KEY_UNNECESSARY,
-      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter2)));
-  ASSERT_TRUE(scheduler()->ScheduleConfiguration(params2));
-  ASSERT_EQ(1, counter2.times_called());
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
-
   // Poll.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))
       .Times(AtLeast(1))
diff --git a/sync/engine/sync_scheduler_whitebox_unittest.cc b/sync/engine/sync_scheduler_whitebox_unittest.cc
index 390a6cd..34ff7d9 100644
--- a/sync/engine/sync_scheduler_whitebox_unittest.cc
+++ b/sync/engine/sync_scheduler_whitebox_unittest.cc
@@ -107,7 +107,6 @@ class SyncSchedulerWhiteboxTest : public testing::Test {
     SyncScheduler::SyncSessionJob job(purpose, TimeTicks::Now(),
          make_linked_ptr(s),
          false,
-         ConfigurationParams(),
          FROM_HERE);
     return DecideOnJob(job);
   }
@@ -161,7 +160,6 @@ TEST_F(SyncSchedulerWhiteboxTest, SaveNudgeWhileTypeThrottled) {
                                     TimeTicks::Now(),
                                     make_linked_ptr(s),
                                     false,
-                                    ConfigurationParams(),
                                     FROM_HERE);
 
   SyncScheduler::JobProcessDecision decision = DecideOnJob(job);