Fix the false-positive detection of commit errors

chrome/browser/sync/sessions/sync_session.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/sessions/sync_session.h"
 
 #include <algorithm>
 
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/model_type.h"
@@ skipping 123 matching lines @@
 }
 
 bool SyncSession::HasMoreToSync() const {
   const StatusController* status = status_controller_.get();
   return ((status->commit_ids().size() < status->unsynced_handles().size()) &&
       status->syncer_status().num_successful_commits > 0) ||
       status->conflict_sets_built() ||
       status->conflicts_resolved();
       // Or, we have conflicting updates, but we're making progress on
       // resolving them...
-  }
+}
+
+bool SyncSession::WasUnableToMakeProgress() const {

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

This name sucks.  I should call it WasAbleToMakeProgress(), because
!WasAbleToMakeProgress() is more readable than WasUnableToMakeProgress().  

+  VLOG(1) << "Was unable to make progress?";
+  const StatusController* status = status_controller_.get();
+
+  // In the future, we should be able to report more about the cause of the
+  // errors and return an enum value.  Right now that's not possible because
+  // our error detection is based on state that can tell us only that an
+  // error exists, but not much else.
+
+  // We were unable to download all updates, for some unknown reason

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

style nit - end comments with periods

+  if (status->num_server_changes_remaining() > 0)
+    return true;
+
+  // Lots of errors could cause this
+  if (status->error().consecutive_errors > 0)

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

Are all these errors "all or nothing"?  I'm wondering how / if we can enforce
that going forward...

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

In the future, I was hoping we could replace this with a GetError() function
that returns an enum value informing us of what kind of error we experienced, or
what the root cause was, or some data structure that contains both.  

Right now I'm not sure how to define an error of any kind.  I also don't know
what the set of error categories would look like.  So I decided to start simple.
 We can base this function on top of GetError() in the future, if we ever figure
out how to implement it.  

+    return true;
+
+  return false;
+}
 
 }  // namespace sessions
 }  // namespace browser_sync