Fix the false-positive detection of commit errors

chrome/browser/sync/sessions/sync_session.cc

Index: chrome/browser/sync/sessions/sync_session.cc
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index f399d7d7904ed1fee8a5425a8641cfaaab04675e..c105ed8cb3e2aea8451888c24a69e345912afc8d 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -141,7 +141,27 @@ bool SyncSession::HasMoreToSync() const {
       status->conflicts_resolved();
       // Or, we have conflicting updates, but we're making progress on
       // resolving them...
-  }
+}
+
+bool SyncSession::ExperiencedTransientError() const {
+  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.
+  if (status->num_server_changes_remaining() > 0)
+    return true;
+
+  // Lots of errors could cause this.  It is believed all of them are
+  // transient.

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

Hm.  Don't we increment this for a number of errors that may not be transient
(e.g. not_my_birthday, the default NOTREACHED() case,  if we get any of 
ClientToServerResponse::ACCESS_DENIED,  ClientToServerResponse::AUTH_INVALID,
ClientToServerResponse::USER_NOT_ACTIVATED, SYNC_AUTH_ERROR)?

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

Maybe so.  I haven't tracked down all the cases that could increment this
counter, nor do I fully understand their implications.  (What does
"not_my_birthday" mean, anyway?)  The main reason I started this code review was
to get feedback on whether or not this if statement makes sense.  

If this does a poor job of detecting transient errors, what alternatives could
we use?  

What should we do when we detect errors that are not transient?  Should we
define another SyncSession function to identify them and add a special case for
them in ScheduleNextSync()?  I've tried to keep the two types of errors separate
because I don't see much point in scheduling a retry job unless there's a chance
the error will go away on its own.

+  if (status->error().consecutive_errors > 0)
+    return true;
+
+  return false;
+}
 
 }  // namespace sessions
 }  // namespace browser_sync