Clean some code in syncscheduler.

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"
@@ skipping 351 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_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);
   } 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,
         old->source(), old->routing_info(), old->workers()));
     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.
 struct ModelSafeWorkerGroupIs {
   explicit ModelSafeWorkerGroupIs(ModelSafeGroup group) : group(group) {}
   bool operator()(ModelSafeWorker* w) {
     return group == w->GetModelSafeGroup();
   }
   ModelSafeGroup group;
 };
@@ skipping 355 matching lines @@
     ModelTypePayloadMap::const_iterator iter;
     for (iter = job.session->source().types.begin();
          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.";
-    // 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);
 }
 
 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
   // but HasMoreToSync is false, this implies that the Syncer determined no
   // forward progress was possible at this time (an error, such as an HTTP
   // 500, is likely to have occurred during commit).
   int num_server_changes_remaining =
       old_job.session->status_controller()->num_server_changes_remaining();
   size_t num_unsynced_handles =
       old_job.session->status_controller()->unsynced_handles().size();
   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);
+  // 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;
   }
 
   // We are in backoff mode and our time did not run out. That means we had
   // a local change, notification from server or a network connection change
   // notification. In any case set had_nudge = true so we dont retry next
   // nudge. Note: we will keep retrying network connection changes though as
   // they are treated as canary jobs. Also we check the mode here because
@@ skipping 272 matching lines @@
 
 #undef SLOG
 
 #undef VLOG_LOC
 
 #undef VLOG_LOC_STREAM
 
 #undef ENUM_CASE
 
 }  // browser_sync