Revert 146262 - Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

sync/engine/sync_scheduler_unittest.cc

Index: sync/engine/sync_scheduler_unittest.cc
===================================================================
--- sync/engine/sync_scheduler_unittest.cc	(revision 146499)
+++ sync/engine/sync_scheduler_unittest.cc	(working copy)
@@ -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::Eq;
 using testing::Invoke;
 using testing::Mock;
-using testing::Not;
 using testing::Return;
 using testing::WithArg;
 
@@ -70,14 +68,6 @@
   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 {
@@ -161,7 +151,7 @@
   }
 
   void StartSyncScheduler(SyncScheduler::Mode mode) {
-    scheduler()->Start(mode);
+    scheduler()->Start(mode, base::Closure());
   }
 
   // This stops the scheduler synchronously.
@@ -300,23 +290,14 @@
   SyncShareRecords records;
   const ModelTypeSet model_types(syncer::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,
@@ -333,9 +314,7 @@
   SyncShareRecords records;
   const ModelTypeSet model_types(syncer::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),
@@ -344,27 +323,61 @@
   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(syncer::BOOKMARKS),
+      model_types2(syncer::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) {
@@ -387,28 +400,19 @@
   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());
-
   scheduler()->ScheduleNudgeAsync(
       zero(), NUDGE_SOURCE_LOCAL, model_types, FROM_HERE);
   RunLoop();
+
   ASSERT_EQ(2U, records.snapshots.size());
-  ASSERT_EQ(0, counter.times_called());
-
   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());
@@ -713,19 +717,13 @@
   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(syncer::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());
 
@@ -737,15 +735,8 @@
 
   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) {
@@ -778,7 +769,6 @@
   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))));
 
@@ -792,15 +782,8 @@
 
   const ModelTypeSet config_types(syncer::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,
@@ -869,7 +852,7 @@
   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(_)).
@@ -911,15 +894,9 @@
 
   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);
 
@@ -1078,49 +1055,27 @@
   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));
   EXPECT_CALL(*syncer(), SyncShare(_, DOWNLOAD_UPDATES, APPLY_UPDATES))
       .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
 
-  syncer::ModelTypeSet model_types(syncer::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);
 
-  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());
+  scheduler()->CleanupDisabledTypes();
+
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
-
   // Poll.
   EXPECT_CALL(*syncer(), SyncShare(_, SYNCER_BEGIN, SYNCER_END))
       .Times(AtLeast(1))