Reland 148792

Relanding after rebasing.

[Sync] Remove CleanupDisabledTypes command and move purge logic into SyncManager."

Original codereview at http://codereview.chromium.org/10541079/

BUG=131433, 90868
TBR=tim@chromium.org


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

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

13 files changed, 185 insertions, 331 deletions

diff --git a/sync/engine/cleanup_disabled_types_command.cc b/sync/engine/cleanup_disabled_types_command.cc
deleted file mode 100644
index d7dc7a6..0000000
--- a/sync/engine/cleanup_disabled_types_command.cc
+++ /dev/null
@@ -1,67 +0,0 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "sync/engine/cleanup_disabled_types_command.h"
-
-#include <algorithm>
-
-#include "sync/internal_api/public/base/model_type.h"
-#include "sync/sessions/sync_session.h"
-#include "sync/sessions/sync_session_context.h"
-#include "sync/syncable/directory.h"
-
-namespace syncer {
-
-CleanupDisabledTypesCommand::CleanupDisabledTypesCommand() {}
-CleanupDisabledTypesCommand::~CleanupDisabledTypesCommand() {}
-
-SyncerError CleanupDisabledTypesCommand::ExecuteImpl(
-    sessions::SyncSession* session) {
-  // Because a full directory purge is slow, we avoid purging
-  // undesired types unless we have reason to believe they were
-  // previously enabled.  Because purging could theoretically fail on
-  // the first sync session (when there's no previous routing info) we
-  // pay the full directory scan price once and do a "deep clean" of
-  // types that may potentially need cleanup so that we converge to
-  // the correct state.
-  //
-  //                          in_previous  |   !in_previous
-  //                                       |
-  //   initial_sync_ended     should clean |  may have attempted cleanup
-  //  !initial_sync_ended     should clean |  may have never been enabled, or
-  //                                       |  could have been disabled before
-  //                                       |  initial sync ended and cleanup
-  //                                       |  may not have happened yet
-  //                                       |  (failure, browser restart
-  //                                       |  before another sync session,..)
-
-  const ModelTypeSet enabled_types =
-      GetRoutingInfoTypes(session->routing_info());
-
-  const ModelTypeSet previous_enabled_types =
-      GetRoutingInfoTypes(
-          session->context()->previous_session_routing_info());
-
-  ModelTypeSet to_cleanup = Difference(ModelTypeSet::All(), enabled_types);
-
-  // If |previous_enabled_types| is non-empty (i.e., not the first
-  // sync session), set |to_cleanup| to its intersection with
-  // |previous_enabled_types|.
-  if (!previous_enabled_types.Empty()) {
-    to_cleanup.RetainAll(previous_enabled_types);
-  }
-
-  DVLOG(1) << "enabled_types = " << ModelTypeSetToString(enabled_types)
-           << ", previous_enabled_types = "
-           << ModelTypeSetToString(previous_enabled_types)
-           << ", to_cleanup = " << ModelTypeSetToString(to_cleanup);
-
-  if (to_cleanup.Empty())
-    return SYNCER_OK;
-
-  session->context()->directory()->PurgeEntriesWithTypeIn(to_cleanup);
-  return SYNCER_OK;
-}
-
-}  // namespace syncer
diff --git a/sync/engine/cleanup_disabled_types_command.h b/sync/engine/cleanup_disabled_types_command.h
deleted file mode 100644
index 2090f1d..0000000
--- a/sync/engine/cleanup_disabled_types_command.h
+++ /dev/null
@@ -1,44 +0,0 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_
-#define SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_
-
-#include "base/compiler_specific.h"
-#include "sync/engine/syncer_command.h"
-
-namespace syncer {
-
-// A syncer command that purges (from memory and disk) entries belonging to
-// a ModelType or ServerModelType that the user has not elected to sync.
-//
-// This is done as part of a session to 1) ensure it does not block the UI,
-// and 2) avoid complicated races that could arise between a) deleting
-// things b) a sync session trying to use these things c) and the potential
-// re-enabling of the data type by the user before some scheduled deletion
-// took place.  Here, we are safe to perform I/O synchronously and we know it
-// is a safe time to delete as we are in the only active session.
-//
-// The removal from memory is done synchronously, while the disk purge is left
-// to an asynchronous SaveChanges operation.  However, all the updates for
-// meta data fields (such as initial_sync_ended) as well as the actual entry
-// deletions will be committed in a single sqlite transaction.  Thus it is
-// possible that disabled types re-appear (in the sync db) after a reboot,
-// but things will remain in a consistent state.  This kind of error case is
-// cared for in this command by retrying; see ExecuteImpl.
-class CleanupDisabledTypesCommand : public SyncerCommand {
- public:
-  CleanupDisabledTypesCommand();
-  virtual ~CleanupDisabledTypesCommand();
-
-  // SyncerCommand implementation.
-  virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(CleanupDisabledTypesCommand);
-};
-
-}  // namespace syncer
-
-#endif  // SYNC_ENGINE_CLEANUP_DISABLED_TYPES_COMMAND_H_
diff --git a/sync/engine/cleanup_disabled_types_command_unittest.cc b/sync/engine/cleanup_disabled_types_command_unittest.cc
deleted file mode 100644
index 286ca8d..0000000
--- a/sync/engine/cleanup_disabled_types_command_unittest.cc
+++ /dev/null
@@ -1,75 +0,0 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <vector>
-
-#include "sync/engine/cleanup_disabled_types_command.h"
-
-#include "sync/internal_api/public/base/model_type_test_util.h"
-#include "sync/sessions/sync_session.h"
-#include "sync/test/engine/syncer_command_test.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-
-namespace {
-
-using testing::_;
-
-class CleanupDisabledTypesCommandTest : public MockDirectorySyncerCommandTest {
- public:
-  CleanupDisabledTypesCommandTest() {}
-
-  virtual void SetUp() {
-    mutable_routing_info()->clear();
-    (*mutable_routing_info())[BOOKMARKS] = GROUP_PASSIVE;
-    MockDirectorySyncerCommandTest::SetUp();
-  }
-};
-
-// TODO(tim): Add syncer test to verify previous routing info is set.
-TEST_F(CleanupDisabledTypesCommandTest, NoPreviousRoutingInfo) {
-  CleanupDisabledTypesCommand command;
-  ModelTypeSet expected = ModelTypeSet::All();
-  expected.Remove(BOOKMARKS);
-  EXPECT_CALL(*mock_directory(),
-              PurgeEntriesWithTypeIn(HasModelTypes(expected)));
-  command.ExecuteImpl(session());
-}
-
-TEST_F(CleanupDisabledTypesCommandTest, NoPurge) {
-  CleanupDisabledTypesCommand command;
-  EXPECT_CALL(*mock_directory(), PurgeEntriesWithTypeIn(_)).Times(0);
-
-  ModelSafeRoutingInfo prev(routing_info());
-  session()->context()->set_previous_session_routing_info(prev);
-  (*mutable_routing_info())[AUTOFILL] = GROUP_PASSIVE;
-  command.ExecuteImpl(session());
-
-  prev = routing_info();
-  command.ExecuteImpl(session());
-}
-
-TEST_F(CleanupDisabledTypesCommandTest, TypeDisabled) {
-  CleanupDisabledTypesCommand command;
-
-  (*mutable_routing_info())[AUTOFILL] = GROUP_PASSIVE;
-  (*mutable_routing_info())[THEMES] = GROUP_PASSIVE;
-  (*mutable_routing_info())[EXTENSIONS] = GROUP_PASSIVE;
-
-  ModelSafeRoutingInfo prev(routing_info());
-  prev[PASSWORDS] = GROUP_PASSIVE;
-  prev[PREFERENCES] = GROUP_PASSIVE;
-  session()->context()->set_previous_session_routing_info(prev);
-
-  const ModelTypeSet expected(PASSWORDS, PREFERENCES);
-  EXPECT_CALL(*mock_directory(),
-              PurgeEntriesWithTypeIn(HasModelTypes(expected)));
-  command.ExecuteImpl(session());
-}
-
-}  // namespace
-
-}  // namespace syncer
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index e02de2c..72f8243 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -132,7 +132,6 @@ const char* SyncSchedulerImpl::SyncSessionJob::GetPurposeString(
     ENUM_CASE(POLL);
     ENUM_CASE(NUDGE);
     ENUM_CASE(CONFIGURATION);
-    ENUM_CASE(CLEANUP_DISABLED_TYPES);
   }
   NOTREACHED();
   return "";
@@ -354,17 +353,6 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
                        &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.
@@ -411,7 +399,6 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
-  DCHECK_NE(job.purpose, SyncSessionJob::CLEANUP_DISABLED_TYPES);
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
@@ -444,8 +431,6 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(
 SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (job.purpose == SyncSessionJob::CLEANUP_DISABLED_TYPES)
-    return CONTINUE;
 
   // See if our type is throttled.
   ModelTypeSet throttled_types =
@@ -539,8 +524,6 @@ bool SyncSchedulerImpl::ShouldRunJob(const SyncSessionJob& job) {
 
 void SyncSchedulerImpl::SaveJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  // TODO(sync): Should we also check that job.purpose !=
-  // CLEANUP_DISABLED_TYPES?  (See http://crbug.com/90868.)
   if (job.purpose == SyncSessionJob::NUDGE) {
     SDVLOG(2) << "Saving a nudge job";
     InitOrCoalescePendingJob(job);
@@ -697,10 +680,6 @@ void SyncSchedulerImpl::SetSyncerStepsForPurpose(
       *start = SYNCER_BEGIN;
       *end = SYNCER_END;
       return;
-    case SyncSessionJob::CLEANUP_DISABLED_TYPES:
-      *start = CLEANUP_DISABLED_TYPES;
-      *end = CLEANUP_DISABLED_TYPES;
-      return;
     default:
       NOTREACHED();
       *start = SYNCER_END;
@@ -805,27 +784,6 @@ void SyncSchedulerImpl::DoSyncSessionJob(const SyncSessionJob& job) {
   FinishSyncSessionJob(job);
 }
 
-void SyncSchedulerImpl::UpdateCarryoverSessionState(
-    const SyncSessionJob& old_job) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
-    // Whatever types were part of a configuration task will have had updates
-    // downloaded.  For that reason, we make sure they get recorded in the
-    // event that they get disabled at a later time.
-    ModelSafeRoutingInfo r(session_context_->previous_session_routing_info());
-    if (!r.empty()) {
-      ModelSafeRoutingInfo temp_r;
-      ModelSafeRoutingInfo old_info(old_job.session->routing_info());
-      std::set_union(r.begin(), r.end(), old_info.begin(), old_info.end(),
-          std::insert_iterator<ModelSafeRoutingInfo>(temp_r, temp_r.begin()));
-      session_context_->set_previous_session_routing_info(temp_r);
-    }
-  } else {
-    session_context_->set_previous_session_routing_info(
-        old_job.session->routing_info());
-  }
-}
-
 void SyncSchedulerImpl::FinishSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   // Update timing information for how often datatypes are triggering nudges.
@@ -855,7 +813,6 @@ void SyncSchedulerImpl::FinishSyncSessionJob(const SyncSessionJob& job) {
   ServerConnectionManager* scm = session_context_->connection_manager();
   UpdateServerConnectionManagerStatus(scm->server_status());
 
-  UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced()) {
     SDVLOG(2) << "We are currently throttled; not scheduling the next sync.";
     // TODO(sync): Investigate whether we need to check job.purpose
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 179f900..c86588a 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -99,12 +99,8 @@ class SyncSchedulerImpl : public SyncScheduler {
       // a sync.  The source is inferable from |session.source()|.
       NUDGE,
       // Typically used for fetching updates for a subset of the enabled types
-      // during initial sync or reconfiguration.  We don't run all steps of
-      // the sync cycle for these (e.g. CleanupDisabledTypes is skipped).
+      // during initial sync or reconfiguration.
       CONFIGURATION,
-      // The user disabled some types and we have to clean up the data
-      // for those.
-      CLEANUP_DISABLED_TYPES,
     };
     SyncSessionJob();
     SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start,
@@ -220,10 +216,6 @@ class SyncSchedulerImpl : public SyncScheduler {
   // reset our state.
   void FinishSyncSessionJob(const SyncSessionJob& job);
 
-  // Record important state that might be needed in future syncs, such as which
-  // data types may require cleanup.
-  void UpdateCarryoverSessionState(const SyncSessionJob& old_job);
-
   // Helper to FinishSyncSessionJob to schedule the next sync operation.
   void ScheduleNextSync(const SyncSessionJob& old_job);
 
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 6d98372..7581c4dc 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -301,9 +301,7 @@ TEST_F(SyncSchedulerTest, Config) {
   SyncShareRecords records;
   const ModelTypeSet model_types(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))));
 
@@ -334,9 +332,7 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
   SyncShareRecords records;
   const ModelTypeSet model_types(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),
@@ -710,19 +706,14 @@ 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(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());
 
@@ -775,7 +766,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))));
 
@@ -900,10 +890,6 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
   EXPECT_EQ(GetUpdatesCallerInfo::LOCAL,
             r.snapshots[1].source().updates_source);
 
-  // Cleanup is not affected by backoff, but it should not relieve it either.
-  EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES))
-      .WillOnce(Invoke(sessions::test_util::SimulateSuccess));
   EXPECT_CALL(*delay(), GetDelay(_)).Times(0);
 
   StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
@@ -1077,10 +1063,7 @@ 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);
@@ -1099,25 +1082,6 @@ TEST_F(SyncSchedulerTest, SyncerSteps) {
   StopSyncScheduler();
   Mock::VerifyAndClearExpectations(syncer());
 
-  // Cleanup disabled types. Because no types are being configured, we just
-  // perform the cleanup.
-  EXPECT_CALL(*syncer(),
-              SyncShare(_, CLEANUP_DISABLED_TYPES, CLEANUP_DISABLED_TYPES)).
-      WillOnce(Invoke(sessions::test_util::SimulateSuccess));
-  StartSyncScheduler(SyncScheduler::CONFIGURATION_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());
-  StopSyncScheduler();
-  Mock::VerifyAndClearExpectations(syncer());
-
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
   // Poll.
@@ -1163,24 +1127,4 @@ TEST_F(SyncSchedulerTest, StartWhenNotConnected) {
   MessageLoop::current()->RunAllPending();
 }
 
-TEST_F(SyncSchedulerTest, SetsPreviousRoutingInfo) {
-  ModelSafeRoutingInfo info;
-  EXPECT_TRUE(info == context()->previous_session_routing_info());
-  ModelSafeRoutingInfo expected(context()->routing_info());
-  ASSERT_FALSE(expected.empty());
-  EXPECT_CALL(*syncer(), SyncShare(_,_,_)).Times(1);
-
-  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
-
-  scheduler()->ScheduleNudgeAsync(
-      zero(), NUDGE_SOURCE_LOCAL, ModelTypeSet(), FROM_HERE);
-  PumpLoop();
-  // Pump again to run job.
-  PumpLoop();
-
-  StopSyncScheduler();
-
-  EXPECT_TRUE(expected == context()->previous_session_routing_info());
-}
-
 }  // namespace syncer
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index e813bc3..66afdbc 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -12,7 +12,6 @@
 #include "build/build_config.h"
 #include "sync/engine/apply_updates_command.h"
 #include "sync/engine/build_commit_command.h"
-#include "sync/engine/cleanup_disabled_types_command.h"
 #include "sync/engine/commit.h"
 #include "sync/engine/conflict_resolver.h"
 #include "sync/engine/download_updates_command.h"
@@ -53,7 +52,6 @@ const char* SyncerStepToString(const SyncerStep step)
 {
   switch (step) {
     ENUM_CASE(SYNCER_BEGIN);
-    ENUM_CASE(CLEANUP_DISABLED_TYPES);
     ENUM_CASE(DOWNLOAD_UPDATES);
     ENUM_CASE(PROCESS_CLIENT_COMMAND);
     ENUM_CASE(VERIFY_UPDATES);
@@ -106,14 +104,8 @@ void Syncer::SyncShare(sessions::SyncSession* session,
             PruneUnthrottledTypes(base::TimeTicks::Now());
         session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
 
-        next_step = CLEANUP_DISABLED_TYPES;
-        break;
-      case CLEANUP_DISABLED_TYPES: {
-        CleanupDisabledTypesCommand cleanup;
-        cleanup.Execute(session);
         next_step = DOWNLOAD_UPDATES;
         break;
-      }
       case DOWNLOAD_UPDATES: {
         // TODO(akalin): We may want to propagate this switch up
         // eventually.
diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h
index 81271ab..b49e978 100644
--- a/sync/engine/syncer.h
+++ b/sync/engine/syncer.h
@@ -26,7 +26,6 @@ class MutableEntry;
 
 enum SyncerStep {
   SYNCER_BEGIN,
-  CLEANUP_DISABLED_TYPES,
   DOWNLOAD_UPDATES,
   PROCESS_CLIENT_COMMAND,
   VERIFY_UPDATES,
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 24d614e..7e173d1 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -338,6 +338,19 @@ void SyncManagerImpl::ConfigureSyncer(
   DCHECK(!ready_task.is_null());
   DCHECK(!retry_task.is_null());
 
+  // Cleanup any types that might have just been disabled.
+  ModelTypeSet previous_types = ModelTypeSet::All();
+  if (!session_context_->routing_info().empty())
+    previous_types = GetRoutingInfoTypes(session_context_->routing_info());
+  if (!PurgeDisabledTypes(previous_types,
+                          GetRoutingInfoTypes(new_routing_info))) {
+    // We failed to cleanup the types. Invoke the ready task without actually
+    // configuring any types. The caller should detect this as a configuration
+    // failure and act appropriately.
+    ready_task.Run();
+    return;
+  }
+
   // 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 =
@@ -689,6 +702,19 @@ bool SyncManagerImpl::PurgePartiallySyncedTypes() {
   return directory()->PurgeEntriesWithTypeIn(partially_synced_types);
 }
 
+bool SyncManagerImpl::PurgeDisabledTypes(
+    ModelTypeSet previously_enabled_types,
+    ModelTypeSet currently_enabled_types) {
+  ModelTypeSet disabled_types = Difference(previously_enabled_types,
+                                           currently_enabled_types);
+  if (disabled_types.Empty())
+    return true;
+
+  DVLOG(1) << "Purging disabled types "
+           << ModelTypeSetToString(disabled_types);
+  return directory()->PurgeEntriesWithTypeIn(disabled_types);
+}
+
 void SyncManagerImpl::UpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK(thread_checker_.CalledOnValidThread());
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index f3a57a1..190f77d 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -179,6 +179,7 @@ class SyncManagerImpl : public SyncManager,
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, NudgeDelayTest);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnNotificationStateChange);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, OnIncomingNotification);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, PurgeDisabledTypes);
 
   struct NotificationInfo {
     NotificationInfo();
diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc
index b563fa3..7ac20d0 100644
--- a/sync/internal_api/syncapi_unittest.cc
+++ b/sync/internal_api/syncapi_unittest.cc
@@ -919,6 +919,22 @@ class SyncManagerTest : public testing::Test,
         REMOTE_NOTIFICATION);
   }
 
+  void SetProgressMarkerForType(ModelType type, bool set) {
+    if (set) {
+      sync_pb::DataTypeProgressMarker marker;
+      marker.set_token("token");
+      marker.set_data_type_id(GetSpecificsFieldNumberFromModelType(type));
+      sync_manager_.directory()->SetDownloadProgress(type, marker);
+    } else {
+      sync_pb::DataTypeProgressMarker marker;
+      sync_manager_.directory()->SetDownloadProgress(type, marker);
+    }
+  }
+
+  void SetInitialSyncEndedForType(ModelType type, bool value) {
+    sync_manager_.directory()->set_initial_sync_ended_for_type(type, value);
+  }
+
  private:
   // Needed by |sync_manager_|.
   MessageLoop message_loop_;
@@ -2502,18 +2518,24 @@ class MockSyncScheduler : public FakeSyncScheduler {
 
 class ComponentsFactory : public TestInternalComponentsFactory {
  public:
-  ComponentsFactory(SyncScheduler* scheduler_to_use)
+  ComponentsFactory(SyncScheduler* scheduler_to_use,
+                    sessions::SyncSessionContext** session_context)
       : TestInternalComponentsFactory(
             TestInternalComponentsFactory::IN_MEMORY),
-        scheduler_to_use_(scheduler_to_use) {}
+        scheduler_to_use_(scheduler_to_use),
+        session_context_(session_context) {}
   virtual ~ComponentsFactory() {}
 
   virtual scoped_ptr<SyncScheduler> BuildScheduler(
       const std::string& name,
       sessions::SyncSessionContext* context) OVERRIDE {
+    *session_context_ = context;
     return scheduler_to_use_.Pass();
   }
+
+ private:
   scoped_ptr<SyncScheduler> scheduler_to_use_;
+  sessions::SyncSessionContext** session_context_;
 };
 
 class SyncManagerTestWithMockScheduler : public SyncManagerTest {
@@ -2521,24 +2543,42 @@ class SyncManagerTestWithMockScheduler : public SyncManagerTest {
   SyncManagerTestWithMockScheduler() : scheduler_(NULL) {}
   virtual InternalComponentsFactory* GetFactory() OVERRIDE {
     scheduler_ = new MockSyncScheduler();
-    return new ComponentsFactory(scheduler_);
+    return new ComponentsFactory(scheduler_, &session_context_);
+  }
+
+  MockSyncScheduler* scheduler() { return scheduler_; }
+  sessions::SyncSessionContext* session_context() {
+      return session_context_;
   }
+
+ private:
   MockSyncScheduler* scheduler_;
+  sessions::SyncSessionContext* session_context_;
 };
 
 // Test that the configuration params are properly created and sent to
-// ScheduleConfigure. No callback should be invoked.
+// ScheduleConfigure. No callback should be invoked. Any disabled datatypes
+// should be purged.
 TEST_F(SyncManagerTestWithMockScheduler, BasicConfiguration) {
   ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
   ModelTypeSet types_to_download(BOOKMARKS, PREFERENCES);
   ModelSafeRoutingInfo new_routing_info;
   GetModelSafeRoutingInfo(&new_routing_info);
+  ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info);
+  ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
 
   ConfigurationParams params;
-  EXPECT_CALL(*scheduler_, Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler_, ScheduleConfiguration(_)).
+  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
 
+  // Set data for all types.
+  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
+       iter.Inc()) {
+    SetProgressMarkerForType(iter.Get(), true);
+    SetInitialSyncEndedForType(iter.Get(), true);
+  }
+
   CallbackCounter ready_task_counter, retry_task_counter;
   sync_manager_.ConfigureSyncer(
       reason,
@@ -2554,6 +2594,70 @@ TEST_F(SyncManagerTestWithMockScheduler, BasicConfiguration) {
             params.source);
   EXPECT_TRUE(types_to_download.Equals(params.types_to_download));
   EXPECT_EQ(new_routing_info, params.routing_info);
+
+  // Verify all the disabled types were purged.
+  EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
+      enabled_types));
+  EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
+      ModelTypeSet::All()).Equals(disabled_types));
+}
+
+// Test that on a reconfiguration (configuration where the session context
+// already has routing info), only those recently disabled types are purged.
+TEST_F(SyncManagerTestWithMockScheduler, ReConfiguration) {
+  ConfigureReason reason = CONFIGURE_REASON_RECONFIGURATION;
+  ModelTypeSet types_to_download(BOOKMARKS, PREFERENCES);
+  ModelTypeSet disabled_types = ModelTypeSet(THEMES, SESSIONS);
+  ModelSafeRoutingInfo old_routing_info;
+  ModelSafeRoutingInfo new_routing_info;
+  GetModelSafeRoutingInfo(&old_routing_info);
+  new_routing_info = old_routing_info;
+  new_routing_info.erase(THEMES);
+  new_routing_info.erase(SESSIONS);
+  ModelTypeSet enabled_types = GetRoutingInfoTypes(new_routing_info);
+
+  ConfigurationParams params;
+  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
+      WillOnce(DoAll(SaveArg<0>(&params), Return(true)));
+
+  // Set data for all types except those recently disabled (so we can verify
+  // only those recently disabled are purged) .
+  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
+       iter.Inc()) {
+    if (!disabled_types.Has(iter.Get())) {
+      SetProgressMarkerForType(iter.Get(), true);
+      SetInitialSyncEndedForType(iter.Get(), true);
+    } else {
+      SetProgressMarkerForType(iter.Get(), false);
+      SetInitialSyncEndedForType(iter.Get(), false);
+    }
+  }
+
+  // Set the context to have the old routing info.
+  session_context()->set_routing_info(old_routing_info);
+
+  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);
+
+  // Verify only the recently disabled types were purged.
+  EXPECT_TRUE(sync_manager_.InitialSyncEndedTypes().Equals(
+      Difference(ModelTypeSet::All(), disabled_types)));
+  EXPECT_TRUE(sync_manager_.GetTypesWithEmptyProgressMarkerToken(
+      ModelTypeSet::All()).Equals(disabled_types));
 }
 
 // Test that the retry callback is invoked on configuration failure.
@@ -2564,8 +2668,8 @@ TEST_F(SyncManagerTestWithMockScheduler, ConfigurationRetry) {
   GetModelSafeRoutingInfo(&new_routing_info);
 
   ConfigurationParams params;
-  EXPECT_CALL(*scheduler_, Start(SyncScheduler::CONFIGURATION_MODE));
-  EXPECT_CALL(*scheduler_, ScheduleConfiguration(_)).
+  EXPECT_CALL(*scheduler(), Start(SyncScheduler::CONFIGURATION_MODE));
+  EXPECT_CALL(*scheduler(), ScheduleConfiguration(_)).
       WillOnce(DoAll(SaveArg<0>(&params), Return(false)));
 
   CallbackCounter ready_task_counter, retry_task_counter;
@@ -2627,4 +2731,44 @@ TEST_F(SyncManagerTest, PurgePartiallySyncedTypes) {
   EXPECT_FALSE(partial_types.Has(PREFERENCES));
 }
 
+// Test CleanipDisabledTypes properly purges all disabled types as specified
+// by the previous and current enabled params. Enabled partial types should not
+// be purged.
+TEST_F(SyncManagerTest, PurgeDisabledTypes) {
+  ModelSafeRoutingInfo routing_info;
+  GetModelSafeRoutingInfo(&routing_info);
+  ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
+  ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
+  ModelTypeSet partial_enabled_types(PASSWORDS);
+
+  // Set data for all non-partial types.
+  for (ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
+       iter.Inc()) {
+    SetProgressMarkerForType(iter.Get(), true);
+    if (!partial_enabled_types.Has(iter.Get()))
+      SetInitialSyncEndedForType(iter.Get(), true);
+  }
+
+  // Verify all the enabled types remain after cleanup, and all the disabled
+  // types were purged.
+  sync_manager_.PurgeDisabledTypes(ModelTypeSet::All(), enabled_types);
+  EXPECT_TRUE(enabled_types.Equals(
+      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
+  EXPECT_TRUE(disabled_types.Equals(
+      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
+
+  // Disable some more types.
+  disabled_types.Put(BOOKMARKS);
+  disabled_types.Put(PREFERENCES);
+  ModelTypeSet new_enabled_types =
+      Difference(ModelTypeSet::All(), disabled_types);
+
+  // Verify only the non-disabled types remain after cleanup.
+  sync_manager_.PurgeDisabledTypes(enabled_types, new_enabled_types);
+  EXPECT_TRUE(new_enabled_types.Equals(
+      Union(sync_manager_.InitialSyncEndedTypes(), partial_enabled_types)));
+  EXPECT_TRUE(disabled_types.Equals(
+      sync_manager_.GetTypesWithEmptyProgressMarkerToken(ModelTypeSet::All())));
+}
+
 }  // namespace
diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h
index cd26d5e..400a6aa 100644
--- a/sync/sessions/sync_session_context.h
+++ b/sync/sessions/sync_session_context.h
@@ -109,14 +109,6 @@ class SyncSessionContext {
   }
   int32 max_commit_batch_size() const { return max_commit_batch_size_; }
 
-  const ModelSafeRoutingInfo& previous_session_routing_info() const {
-    return previous_session_routing_info_;
-  }
-
-  void set_previous_session_routing_info(const ModelSafeRoutingInfo& info) {
-    previous_session_routing_info_ = info;
-  }
-
   void NotifyListeners(const SyncEngineEvent& event) {
     FOR_EACH_OBSERVER(SyncEngineEventListener, listeners_,
                       OnSyncEngineEvent(event));
@@ -162,10 +154,6 @@ class SyncSessionContext {
   // The server limits the number of items a client can commit in one batch.
   int max_commit_batch_size_;
 
-  // Some routing info history to help us clean up types that get disabled
-  // by the user.
-  ModelSafeRoutingInfo previous_session_routing_info_;
-
   ThrottledDataTypeTracker* throttled_data_type_tracker_;
 
   // We use this to get debug info to send to the server for debugging
diff --git a/sync/sync.gyp b/sync/sync.gyp
index ea2f7b3..8ca2c19 100644
--- a/sync/sync.gyp
+++ b/sync/sync.gyp
@@ -76,8 +76,6 @@
         'engine/apply_updates_command.h',
         'engine/build_commit_command.cc',
         'engine/build_commit_command.h',
-        'engine/cleanup_disabled_types_command.cc',
-        'engine/cleanup_disabled_types_command.h',
         'engine/commit.cc',
         'engine/commit.h',
         'engine/conflict_resolver.cc',
@@ -561,7 +559,6 @@
           'internal_api/public/util/immutable_unittest.cc',
           'engine/apply_updates_command_unittest.cc',
           'engine/build_commit_command_unittest.cc',
-          'engine/cleanup_disabled_types_command_unittest.cc',
           'engine/download_updates_command_unittest.cc',
           'engine/model_changing_syncer_command_unittest.cc',
           'engine/process_commit_response_command_unittest.cc',