Fix the false-positive detection of commit errors

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 795 matching lines @@
     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());
-  // 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();
     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
@@ skipping 305 matching lines @@
 
 #undef SVLOG_LOC
 
 #undef SVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // browser_sync