Clean some code in syncscheduler.

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 82a847639a2f6d017ac842951f1abc5b7e4e08f1..9c40b478ba7710c96ae2d6082ec2c1bd0145f44b 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -369,8 +369,7 @@ bool SyncScheduler::ShouldRunJob(const SyncSessionJob& job) {
 void SyncScheduler::SaveJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK_NE(job.purpose, SyncSessionJob::CLEAR_USER_DATA);
-  // TODO(sync): Should we also check that job.purpose !=
-  // CLEANUP_DISABLED_TYPES?  (See http://crbug.com/90868.)
+  DCHECK_NE(job.purpose, SyncSessionJob::CLEANUP_DISABLED_TYPES);
   if (job.purpose == SyncSessionJob::NUDGE) {
     SVLOG(2) << "Saving a nudge job";
     InitOrCoalescePendingJob(job);
@@ -385,10 +384,9 @@ void SyncScheduler::SaveJob(const SyncSessionJob& job) {
     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
-  // SaveJob() only does what it says sometimes.  (See
-  // http://crbug.com/90868.)
+  } else {
+    NOTREACHED();

[tim (not reviewing), 2011/08/12 20:52:40]

a comment or << " ... " would be useful

[lipalani1, 2011/08/12 21:01:47]

Done.

+  }
 }
 
 // Functor for std::find_if to search by ModelSafeGroup.
@@ -764,13 +762,6 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
   }
   last_sync_session_end_time_ = now;
   UpdateCarryoverSessionState(job);
-  if (IsSyncingCurrentlySilenced()) {
-    SVLOG(2) << "We are currently throttled; not scheduling the next sync.";
-    // TODO(sync): Investigate whether we need to check job.purpose
-    // here; see DCHECKs in SaveJob().  (See http://crbug.com/90868.)
-    SaveJob(job);
-    return;  // Nothing to do.
-  }
 
   SVLOG(2) << "Updating the next polling time after SyncMain";
   ScheduleNextSync(job);
@@ -779,6 +770,21 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
 void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!old_job.session->HasMoreToSync());
+
+  // First check for one off jobs like CLEANUP_DISABLED_TYPES. Success or
+  // failure of that job should not affect our states like exponential backoff,
+  // pending_job etc. Also those "one off" jobs dont need to be retried.

[tim (not reviewing), 2011/08/12 20:52:40]

this comment makes it seem as though we don't want them to be reliable?  see
crbug.com/57360.

[lipalani1, 2011/08/12 21:01:47]

So the only other command that needs to be saved is clear user data. Nudges and
configs are taken care of.
There is not enough value in retrying cleanupdisabled types if we find it was
not cleaned properly.
I have another patch for Clear user data which should take care of retrying it.
Thats why I have left it as a TODO here rather than just filling it in.
I am treating CLEAR USER DATA is not a supported feature for now. let me know
when you folks want that in and I can dust the CR and rebase it and send it
again.
On 2011/08/12 20:52:40, timsteele wrote:

[tim (not reviewing), 2011/08/12 21:23:25]

Hm, I'm worried what it would mean for migration if CleanupDisabled fails and we
don't retry?

+  // TODO(lipalani) : handle the case of CLEAR_USER_DATA here.
+  if (old_job.purpose == SyncSessionJob::CLEANUP_DISABLED_TYPES)
+    return;
+
+  // Now check for throttled.
+  if (IsSyncingCurrentlySilenced()) {
+    SVLOG(2) << "We are currently throttled; not scheduling the next sync.";
+    SaveJob(old_job);
+    return;  // Nothing more to schedule.
+  } // Now continue to check if we need to do exponential backoff.
+
   // Note: |num_server_changes_remaining| > 0 here implies that we received a
   // broken response while trying to download all updates, because the Syncer
   // will loop until this value is exhausted. Also, if unsynced_handles exist
@@ -796,15 +802,15 @@ void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
            << ", syncer has work to do: " << work_to_do;
 
   AdjustPolling(&old_job);
+  // Note one off jobs like
+  // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}) would
+  // have been handled by now. We should either be in a nudge or config.
+  DCHECK(old_job.purpose == SyncSessionJob::NUDGE ||
+         old_job.purpose == SyncSessionJob::CONFIGURATION);
 
   // 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,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.
+    // Success implies backoff relief.
     wait_interval_.reset();
     SVLOG(2) << "Job succeeded so not scheduling more jobs";
     return;