[Sync] Add RequestCleanupDisabledTypes() method to SyncManager

chrome/browser/sync/engine/sync_scheduler.cc

Index: chrome/browser/sync/engine/sync_scheduler.cc
diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc
index f87d83a003bf2140eb7f83d9a0fa98e624e4f4d3..d11e884ebeba8de6554c60aa3cc74bc559e4603d 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -13,6 +13,7 @@
 #include "base/rand_util.h"
 #include "base/tracked.h"
 #include "chrome/browser/sync/engine/syncer.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/protocol/proto_enum_conversions.h"
 #include "chrome/browser/sync/util/logging.h"
 
@@ -75,6 +76,7 @@ const char* SyncScheduler::SyncSessionJob::GetPurposeString(
     ENUM_CASE(NUDGE);
     ENUM_CASE(CLEAR_USER_DATA);
     ENUM_CASE(CONFIGURATION);
+    ENUM_CASE(CLEANUP_DISABLED_TYPES);
   }
   NOTREACHED();
   return "";
@@ -252,6 +254,7 @@ SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval(
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
   DCHECK_NE(job.purpose, SyncSessionJob::CLEAR_USER_DATA);
+  DCHECK_NE(job.purpose, SyncSessionJob::CLEANUP_DISABLED_TYPES);
 
   SVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
            << WaitInterval::GetModeString(wait_interval_->mode)
@@ -262,7 +265,7 @@ SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval(
     return DROP;
 
   DCHECK(job.purpose == SyncSessionJob::NUDGE ||
-      job.purpose == SyncSessionJob::CONFIGURATION);
+         job.purpose == SyncSessionJob::CONFIGURATION);
   if (wait_interval_->mode == WaitInterval::THROTTLED)
     return SAVE;
 
@@ -282,7 +285,8 @@ SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval(
 SyncScheduler::JobProcessDecision SyncScheduler::DecideOnJob(
     const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (job.purpose == SyncSessionJob::CLEAR_USER_DATA)
+  if (job.purpose == SyncSessionJob::CLEAR_USER_DATA ||
+      job.purpose == SyncSessionJob::CLEANUP_DISABLED_TYPES)
     return CONTINUE;
 
   if (wait_interval_.get())
@@ -357,7 +361,8 @@ bool SyncScheduler::ShouldRunJob(const SyncSessionJob& job) {
 
 void SyncScheduler::SaveJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  DCHECK(job.purpose != SyncSessionJob::CLEAR_USER_DATA);
+  DCHECK_NE(job.purpose, SyncSessionJob::CLEAR_USER_DATA);
+  DCHECK_NE(job.purpose, SyncSessionJob::CLEANUP_DISABLED_TYPES);
   if (job.purpose == SyncSessionJob::NUDGE) {
     SVLOG(2) << "Saving a nudge job";
     InitOrCoalescePendingJob(job);
@@ -391,6 +396,13 @@ void SyncScheduler::ScheduleClearUserData() {
                &SyncScheduler::ScheduleClearUserDataImpl));
 }
 
+void SyncScheduler::ScheduleCleanupDisabledTypes() {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  PostTask(FROM_HERE, "ScheduleCleanupDisabledTypes",
+           method_factory_.NewRunnableMethod(
+               &SyncScheduler::ScheduleCleanupDisabledTypesImpl));
+}
+
 void SyncScheduler::ScheduleNudge(
     const TimeDelta& delay,
     NudgeSource source, const ModelTypeBitSet& types,
@@ -430,11 +442,16 @@ void SyncScheduler::ScheduleNudgeWithPayloads(
 
 void SyncScheduler::ScheduleClearUserDataImpl() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  SyncSession* session = new SyncSession(session_context_.get(), this,
-      SyncSourceInfo(), ModelSafeRoutingInfo(),
-      std::vector<ModelSafeWorker*>());
-  ScheduleSyncSessionJob(TimeDelta::FromSeconds(0),
-      SyncSessionJob::CLEAR_USER_DATA, session, FROM_HERE);
+  ScheduleSyncSessionJob(
+      TimeDelta::FromSeconds(0), SyncSessionJob::CLEAR_USER_DATA,
+      CreateSyncSession(SyncSourceInfo()), FROM_HERE);
+}
+
+void SyncScheduler::ScheduleCleanupDisabledTypesImpl() {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  ScheduleSyncSessionJob(
+      TimeDelta::FromSeconds(0), SyncSessionJob::CLEANUP_DISABLED_TYPES,
+      CreateSyncSession(SyncSourceInfo()), FROM_HERE);
 }
 
 void SyncScheduler::ScheduleNudgeImpl(
@@ -452,9 +469,6 @@ void SyncScheduler::ScheduleNudgeImpl(
       << syncable::ModelTypePayloadMapToString(types_with_payloads)
       << (is_canary_job ? " (canary)" : "");
 
-  // Note we currently nudge for all types regardless of the ones incurring
-  // the nudge.  Doing different would throw off some syncer commands like
-  // CleanupDisabledTypes.  We may want to change this in the future.
   SyncSourceInfo info(source, types_with_payloads);
 
   SyncSession* session(CreateSyncSession(info));
@@ -631,7 +645,6 @@ void SyncScheduler::SetSyncerStepsForPurpose(
     SyncSessionJob::SyncSessionJobPurpose purpose,
     SyncerStep* start, SyncerStep* end) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  *end = SYNCER_END;
   switch (purpose) {
     case SyncSessionJob::CONFIGURATION:
       *start = DOWNLOAD_UPDATES;
@@ -639,13 +652,22 @@ void SyncScheduler::SetSyncerStepsForPurpose(
       return;
     case SyncSessionJob::CLEAR_USER_DATA:
       *start = CLEAR_PRIVATE_DATA;
+      *end = CLEAR_PRIVATE_DATA;
        return;
     case SyncSessionJob::NUDGE:
     case SyncSessionJob::POLL:
       *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;
+      *end = SYNCER_END;
+      return;
   }
 }
 
@@ -677,7 +699,7 @@ void SyncScheduler::DoSyncSessionJob(const SyncSessionJob& job) {
   SVLOG(2) << "DoSyncSessionJob with "
            << SyncSessionJob::GetPurposeString(job.purpose) << " job";
 
-  SyncerStep begin(SYNCER_BEGIN);
+  SyncerStep begin(SYNCER_END);
   SyncerStep end(SYNCER_END);
   SetSyncerStepsForPurpose(job.purpose, &begin, &end);
 
@@ -691,6 +713,7 @@ void SyncScheduler::DoSyncSessionJob(const SyncSessionJob& job) {
       job.session->ResetTransientState();
   }
   SVLOG(2) << "Done SyncShare looping.";
+
   FinishSyncSessionJob(job);
 }
 
@@ -732,7 +755,10 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
   UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced()) {
     SVLOG(2) << "We are currently throttled; not scheduling the next sync.";
-    SaveJob(job);
+    if (job.purpose != SyncSessionJob::CLEAR_USER_DATA &&
+        job.purpose != SyncSessionJob::CLEANUP_DISABLED_TYPES) {

[tim (not reviewing), 2011/07/28 04:10:40]

hmm.. are you sure about this?  See
http://code.google.com/p/chromium/issues/detail?id=71616  and
http://code.google.com/p/chromium/issues/detail?id=57360

[akalin, 2011/07/28 06:36:45]

Well, we'll hit the DCHECK otherwise.  And even if we pass that, it'll just get
dropped anyway, since only configures and nudges are saved.

At least for cleanup disabled types, saving the job doesn't make sense as it's
not affected by the wait interval and doesn't communicate with the server.

Those issues are valid, but I don't think this CL is the one to fix them,
especially since ClearUserData isn't even hooked up to anything.  I don't know
if we should even keep that code around.

[tim (not reviewing), 2011/07/28 17:28:04]

Well, my point is that one of the main things we did last quarter was make all
sync jobs reliable, so that we can make a blanket simplifying assumption that
we'll never drop jobs. It seems wrong to intentionally regress something by
inspection that we just fixed not that long ago :) 

Maybe I'm misremembering, so to be on the safe side I'll cc Lingesh. 

[akalin, 2011/07/28 17:50:28]

Replaced with TODOs, PTAL.

+      SaveJob(job);
+    }
     return;  // Nothing to do.
   }
 
@@ -763,10 +789,12 @@ void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
 
   // TODO(tim): Old impl had special code if notifications disabled. Needed?
   if (!work_to_do) {
-    // Success implies backoff relief.  Note that if this was a "one-off" job
-    // (i.e. purpose == SyncSessionJob::CLEAR_USER_DATA), if there was
-    // work_to_do before it ran this wont have changed, as jobs like this don't
-    // run a full sync cycle.  So we don't need special code here.
+    // Success implies backoff relief.  Note that if this was a
+    // "one-off" job (i.e. purpose ==
+    // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}), if
+    // there was work_to_do before it ran this wont have changed, as
+    // jobs like this don't run a full sync cycle.  So we don't need
+    // special code here.
     wait_interval_.reset();
     SVLOG(2) << "Job succeeded so not scheduling more jobs";
     return;