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..d9896ec8b7a03e95ae311d70f900c4bf9f81a4c1 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,12 @@ 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 {
+    // Save job should only be called with nudges or configs which need to be
+    // saved. Others jobs are one off jobs which dont need to be retried so
+    // dont need to be saved.
+    NOTREACHED();
+  }
 }
 
 // Functor for std::find_if to search by ModelSafeGroup.
@@ -764,13 +765,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 +773,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.
+  // 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 +805,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;