[Sync] Add RequestCleanupDisabledTypes() method to SyncManager

chrome/browser/sync/engine/sync_scheduler.cc

 // Copyright (c) 2011 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 "chrome/browser/sync/engine/sync_scheduler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #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"
 
 using base::TimeDelta;
 using base::TimeTicks;
 
 namespace browser_sync {
 
 using sessions::SyncSession;
 using sessions::SyncSessionSnapshot;
@@ skipping 42 matching lines @@
 }
 
 const char* SyncScheduler::SyncSessionJob::GetPurposeString(
     SyncScheduler::SyncSessionJob::SyncSessionJobPurpose purpose) {
   switch (purpose) {
     ENUM_CASE(UNKNOWN);
     ENUM_CASE(POLL);
     ENUM_CASE(NUDGE);
     ENUM_CASE(CLEAR_USER_DATA);
     ENUM_CASE(CONFIGURATION);
+    ENUM_CASE(CLEANUP_DISABLED_TYPES);
   }
   NOTREACHED();
   return "";
 }
 
 TimeDelta SyncScheduler::DelayProvider::GetDelay(
     const base::TimeDelta& last_delay) {
   return SyncScheduler::GetRecommendedDelay(last_delay);
 }
 
@@ skipping 157 matching lines @@
   // We just changed our mode. See if there are any pending jobs that we could
   // execute in the new mode.
   DoPendingJobIfPossible(false);
 }
 
 SyncScheduler::JobProcessDecision SyncScheduler::DecideWhileInWaitInterval(
     const SyncSessionJob& job) {
   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)
            << (wait_interval_->had_nudge ? " (had nudge)" : "")
            << (job.is_canary_job ? " (canary)" : "");
 
   if (job.purpose == SyncSessionJob::POLL)
     return DROP;
 
   DCHECK(job.purpose == SyncSessionJob::NUDGE ||
-      job.purpose == SyncSessionJob::CONFIGURATION);
+         job.purpose == SyncSessionJob::CONFIGURATION);
   if (wait_interval_->mode == WaitInterval::THROTTLED)
     return SAVE;
 
   DCHECK_EQ(wait_interval_->mode, WaitInterval::EXPONENTIAL_BACKOFF);
   if (job.purpose == SyncSessionJob::NUDGE) {
     if (mode_ == CONFIGURATION_MODE)
       return SAVE;
 
     // If we already had one nudge then just drop this nudge. We will retry
     // later when the timer runs out.
     return wait_interval_->had_nudge ? DROP : CONTINUE;
   }
   // This is a config job.
   return job.is_canary_job ? CONTINUE : SAVE;
 }
 
 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())
     return DecideWhileInWaitInterval(job);
 
   if (mode_ == CONFIGURATION_MODE) {
     if (job.purpose == SyncSessionJob::NUDGE)
       return SAVE;
     else if (job.purpose == SyncSessionJob::CONFIGURATION)
       return CONTINUE;
@@ skipping 54 matching lines @@
 
   DCHECK(job.purpose == SyncSessionJob::NUDGE || job.purpose ==
       SyncSessionJob::CONFIGURATION);
 
   SaveJob(job);
   return false;
 }
 
 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);
   } else if (job.purpose == SyncSessionJob::CONFIGURATION){
     SVLOG(2) << "Saving a configuration job";
     DCHECK(wait_interval_.get());
     DCHECK(mode_ == CONFIGURATION_MODE);
 
     SyncSession* old = job.session.get();
     SyncSession* s(new SyncSession(session_context_.get(), this,
@@ skipping 13 matching lines @@
   ModelSafeGroup group;
 };
 
 void SyncScheduler::ScheduleClearUserData() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   PostTask(FROM_HERE, "ScheduleClearUserDataImpl",
            method_factory_.NewRunnableMethod(
                &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,
     const tracked_objects::Location& nudge_location) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SVLOG_LOC(nudge_location, 2)
       << "Nudge scheduled with delay " << delay.InMilliseconds() << " ms, "
       << "source " << GetNudgeSourceString(source) << ", "
       << "types " << syncable::ModelTypeBitSetToString(types);
 
@@ skipping 19 matching lines @@
 
   PostTask(nudge_location, "ScheduleNudgeImpl",
            method_factory_.NewRunnableMethod(
                &SyncScheduler::ScheduleNudgeImpl, delay,
                GetUpdatesFromNudgeSource(source), types_with_payloads, false,
                nudge_location));
 }
 
 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(
     const TimeDelta& delay,
     GetUpdatesCallerInfo::GetUpdatesSource source,
     const ModelTypePayloadMap& types_with_payloads,
     bool is_canary_job, const tracked_objects::Location& nudge_location) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   SVLOG_LOC(nudge_location, 2)
       << "In ScheduleNudgeImpl with delay "
       << delay.InMilliseconds() << " ms, "
       << "source " << GetUpdatesSourceString(source) << ", "
       << "payloads "
       << 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));
   SyncSessionJob job(SyncSessionJob::NUDGE, TimeTicks::Now() + delay,
                      make_linked_ptr(session), is_canary_job,
                      nudge_location);
 
   session = NULL;
   if (!ShouldRunJob(job))
     return;
@@ skipping 156 matching lines @@
   PostDelayedTask(from_here, "DoSyncSessionJob",
                   method_factory_.NewRunnableMethod(
                       &SyncScheduler::DoSyncSessionJob, job),
                   delay.InMilliseconds());
 }
 
 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;
       *end = APPLY_UPDATES;
       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;
   }
 }
 
 void SyncScheduler::DoSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (!ShouldRunJob(job)) {
     SLOG(WARNING)
         << "Not executing "
         << SyncSessionJob::GetPurposeString(job.purpose) << " job from "
         << GetUpdatesSourceString(job.session->source().updates_source);
@@ skipping 11 matching lines @@
 
     // Create the session with the latest model safe table and use it to purge
     // and update any disabled or modified entries in the job.
     scoped_ptr<SyncSession> session(CreateSyncSession(job.session->source()));
 
     job.session->RebaseRoutingInfoWithLatest(session.get());
   }
   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);
 
   bool has_more_to_sync = true;
   while (ShouldRunJob(job) && has_more_to_sync) {
     SVLOG(2) << "Calling SyncShare.";
     // Synchronously perform the sync session from this thread.
     syncer_->SyncShare(job.session.get(), begin, end);
     has_more_to_sync = job.session->HasMoreToSync();
     if (has_more_to_sync)
       job.session->ResetTransientState();
   }
   SVLOG(2) << "Done SyncShare looping.";
+
   FinishSyncSessionJob(job);
 }
 
 void SyncScheduler::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.
@@ skipping 21 matching lines @@
          iter != job.session->source().types.end();
          ++iter) {
       syncable::PostTimeToTypeHistogram(iter->first,
                                         now - last_sync_session_end_time_);
     }
   }
   last_sync_session_end_time_ = now;
   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.
   }
 
   SVLOG(2) << "Updating the next polling time after SyncMain";
   ScheduleNextSync(job);
 }
 
 void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!old_job.session->HasMoreToSync());
@@ skipping 10 matching lines @@
   const bool work_to_do =
       num_server_changes_remaining > 0 || num_unsynced_handles > 0;
   SVLOG(2) << "num server changes remaining: " << num_server_changes_remaining
            << ", num unsynced handles: " << num_unsynced_handles
            << ", syncer has work to do: " << work_to_do;
 
   AdjustPolling(&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;
   }
 
   if (old_job.session->source().updates_source ==
       GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION) {
     SVLOG(2) << "Job failed with source continuation";
     // We don't seem to have made forward progress. Start or extend backoff.
     HandleConsecutiveContinuationError(old_job);
@@ skipping 262 matching lines @@
 
 #undef SLOG
 
 #undef VLOG_LOC
 
 #undef VLOG_LOC_STREAM
 
 #undef ENUM_CASE
 
 }  // browser_sync