Avoid SYNC_CYCLE_CONTINUATION sync cycles

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 241692eec05dd64cbd6a9f5dc61250e460bcd51c..8ec74ade1f17cee66eafee26a3afd8247ff76a3a 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -838,30 +838,14 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
 void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!old_job.session->HasMoreToSync());
-  // 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;
-  SDVLOG(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?

[tim (not reviewing), 2012/01/12 01:42:31]

Curious, did you look this up? IIRC there was a chunk of code that I basically
just completely ignored when writing the original SyncScheduler.

[rlarocque, 2012/01/12 19:43:34]

I haven't looked at it too much.  I've seen the code, but it references a lot of
variables that don't mean a lot to me.

That comment was written a year ago.  If removing that code caused bugs, we
probably would have heard about it by now.

-  if (!work_to_do) {
+  if (old_job.session->Succeeded()) {
     // 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
+    // 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();
@@ -869,6 +853,12 @@ void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
     return;
   }
 
+  // TODO(rlarocque): There's no reason why we should blindly backoff and retry
+  // if we don't succeed.  Some types of errors are not likely to disappear on
+  // their own.  With the return values now available in the old_job.session, we
+  // should be able to detect such errors and only retry when we detect
+  // transient errors.
+
   // 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