Fix the false-positive detection of commit errors

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 14991e637c43101562c12670e64137edbfe420ca..3df802bc471d680278bbad9169a2cf5a0733e632 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -813,30 +813,15 @@ 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;
-  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) {
+  if (!old_job.session->ExperiencedTransientError()) {

[tim (not reviewing), 2011/09/27 21:20:07]

We should add a comment here along the lines of if new changes occurred while
this session was in progress, a new nudge should have been posted to the sync
loop, but not yet handled by the scheduler (so the start_time has not yet been
recorded).  Hence, we just check if there we should try to continue syncing due
to a transient error.

[rlarocque, 2011/09/27 23:42:41]

Is this to prevent readers from assuming that this function has something to do
with scheduling nudges that were requested while the previous sync cycle was in
progress?  I don't see why that would be necessary.  There are plenty of reasons
why this function can not and should not deal with changes that happened during
the previous sync cycle.  I don't see why someone would assume that this method
would deal with anything other than a rescheduling of the recently completed
sync cycle.  

Maybe the name ScheduleNextSync is misleading.  If that's the case, we should
fix this by renaming and simplifying the function, not adding comments.  

     // 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();