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 d7d68abd73e6c7a41f0efdd76c473e8f44d69de3..1a922a658391981e42e0a3033686e2d795d63daf 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->WasUnableToMakeProgress()) {

[tim (not reviewing), 2011/09/20 16:21:57]

With the old "SyncerThread" this would have been broken as we would have dropped
nudges queued up while this session was in progress due to the freshness
condition check (which we still have in ShouldRunJob).  But since we'll now only
compute the scheduled_start time at ScheduleNudgeImpl *and* we always coalesce,
I think we're okay.  The freshness check was only useful if you could have
multiple nudges queued up (and we tried to implement it that way with the
scheduler, but for various reasons it didn't fly.)

The condition may still be used for the POLL path, but I'm thinking it is
useless for nudges now; it has to be useless if we know we do not want to check
for unsynced items here.

[rlarocque, 2011/09/20 20:10:03]

I don't follow.  How does this change affect the way we handle queued nudges?  

[tim (not reviewing), 2011/09/20 20:32:12]

I don't think it does. This was to encourage you to look at the code in
ShouldRunJob and consider how this change might affect the |freshness_condition|
check. Because if a change occurs while this session is in progress, and a nudge
is scheduled but we drop that nudge due to the freshness condition, then your
change would break that flow.

[lipalani1, 2011/09/20 23:15:56]

When this job is running no new job(nudge) can come in as they all are in the
same thread.(even when they were in different threads the behaviour would be the
same but complicated to explain :))
So this is the right behavior and ShouldRunJob assumes this behavior. But
Richard should look at it just for fun!
On 2011/09/20 20:32:12, timsteele wrote:

[lipalani1, 2011/09/20 23:18:09]

One more thing, an aside(for Richard): Nudge collapsing is useful when you want
to sync something in x seconds from now and many requests come in that period.
In which case we collapse everything and execute at the xth second.

However when a job has started we dont collapase anything after.
On 2011/09/20 23:15:56, lipalani1 wrote:

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