[Sync] Fix sync scheduler/session logic determining successful commands.

We now have Succeeded(), which is true iff the command ran successfully, and
SuccessfullyReachedServer(), which is true iff we actually contacted the server
without errors.

BUG=131414
TEST=unit_tests


Review URL: https://chromiumcodereview.appspot.com/10537032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@140885 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 55 insertions, 13 deletions

diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 85ac8cc..e219412 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -59,7 +59,8 @@ SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate,
       delegate_(delegate),
       workers_(workers),
       routing_info_(routing_info),
-      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)) {
+      enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)),
+      finished_(false) {
   status_controller_.reset(new StatusController(routing_info_));
   std::sort(workers_.begin(), workers_.end());
 }
@@ -127,6 +128,7 @@ void SyncSession::RebaseRoutingInfoWithLatest(const SyncSession& session) {
 }
 
 void SyncSession::PrepareForAnotherSyncCycle() {
+  finished_ = false;
   source_.updates_source =
       sync_pb::GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION;
   status_controller_.reset(new StatusController(routing_info_));
@@ -226,21 +228,41 @@ namespace {
 // successfully.
 //
 bool IsError(SyncerError error) {
-  return error != UNSET
-      && error != SYNCER_OK;
+  return error != UNSET && error != SYNCER_OK;
 }
-}  // namespace
 
-bool SyncSession::Succeeded() const {
+// Returns false iff one of the command results had an error.
+bool HadErrors(const ErrorCounters& error) {
   const bool download_updates_error =
-      IsError(status_controller_->error().last_download_updates_result);
-  const bool post_commit_error =
-      IsError(status_controller_->error().last_post_commit_result);
+      IsError(error.last_download_updates_result);
+  const bool post_commit_error = IsError(error.last_post_commit_result);
   const bool process_commit_response_error =
-      IsError(status_controller_->error().last_process_commit_response_result);
-  return !download_updates_error
-      && !post_commit_error
-      && !process_commit_response_error;
+      IsError(error.last_process_commit_response_result);
+  return download_updates_error ||
+         post_commit_error ||
+         process_commit_response_error;
+}
+}  // namespace
+
+bool SyncSession::Succeeded() const {
+  const ErrorCounters& error = status_controller_->error();
+  return finished_ && !HadErrors(error);
+}
+
+bool SyncSession::SuccessfullyReachedServer() const {
+  const ErrorCounters& error = status_controller_->error();
+  bool reached_server = error.last_download_updates_result == SYNCER_OK ||
+                        error.last_post_commit_result == SYNCER_OK ||
+                        error.last_process_commit_response_result == SYNCER_OK;
+  // It's possible that we reached the server on one attempt, then had an error
+  // on the next (or didn't perform some of the server-communicating commands).
+  // We want to verify that, for all commands attempted, we successfully spoke
+  // with the server. Therefore, we verify no errors and at least one SYNCER_OK.
+  return reached_server && !HadErrors(error);
+}
+
+void SyncSession::SetFinished() {
+  finished_ = true;
 }
 
 }  // namespace sessions
diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h
index 94148d1..8b15d35 100644
--- a/sync/sessions/sync_session.h
+++ b/sync/sessions/sync_session.h
@@ -113,7 +113,7 @@ class SyncSession {
   // engine again.
   bool HasMoreToSync() const;
 
-  // Returns true if there we did not detect any errors in this session.
+  // Returns true if we completely ran the session without errors.
   //
   // There are many errors that could prevent a sync cycle from succeeding.
   // These include invalid local state, inability to contact the server,
@@ -129,6 +129,10 @@ class SyncSession {
   // since the last call to SyncShare.
   bool Succeeded() const;
 
+  // Returns true if we reached the server successfully and the server did not
+  // return any error codes. Returns false if no connection was attempted.
+  bool SuccessfullyReachedServer() const;
+
   // Collects all state pertaining to how and why |s| originated and unions it
   // with corresponding state in |this|, leaving |s| unchanged.  Allows |this|
   // to take on the responsibilities |s| had (e.g. certain data types) in the
@@ -177,6 +181,9 @@ class SyncSession {
   // Returns the set of enabled groups that have verified updates.
   std::set<ModelSafeGroup> GetEnabledGroupsWithVerifiedUpdates() const;
 
+  // Mark the session has having finished all the sync steps it needed.
+  void SetFinished();
+
  private:
   // Extend the encapsulation boundary to utilities for internal member
   // assignments. This way, the scope of these actions is explicit, they can't
@@ -214,6 +221,10 @@ class SyncSession {
   // |routing_info_|.
   std::set<ModelSafeGroup> enabled_groups_;
 
+  // Whether this session has reached its last step or not. Gets reset on each
+  // new cycle (via PrepareForAnotherSyncCycle).
+  bool finished_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSession);
 };
 
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 0b1ad24..701072e 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -38,6 +38,15 @@ void SimulateSuccess(sessions::SyncSession* session,
     ADD_FAILURE() << "Shouldn't have more to sync";
   }
   ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining());
+  session->SetFinished();
+  if (end == SYNCER_END) {
+    session->mutable_status_controller()->set_last_download_updates_result(
+        SYNCER_OK);
+    session->mutable_status_controller()->set_last_post_commit_result(
+        SYNCER_OK);
+    session->mutable_status_controller()->
+        set_last_process_commit_response_result(SYNCER_OK);
+  }
 }
 
 void SimulateThrottledImpl(sessions::SyncSession* session,