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::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