Refactor following sync commit loop change

This change includes some cleanups of the code introduced in r139519.
They have been kept separate from that CL in the hopes of making both
CLs easiser to read.

This commit moves some error-detection functionality from
ProcessCommitResponse's ModelNeutralExecuteImpl() into
BuildAndPostCommits().  This simplifies some of the error handling and
allows us to remove ModelChangingSyncerCommand's
ModelNeutralExecuteImpl().

This CL also combines both commit error indicators into a single
variable.

BUG=91696,36594
TEST=


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

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

16 files changed, 96 insertions, 141 deletions

diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc
index f384c18..974cd37 100644
--- a/sync/engine/build_commit_command.cc
+++ b/sync/engine/build_commit_command.cc
@@ -124,11 +124,8 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
         static_cast<SyncEntity*>(commit_message->add_entries());
     sync_entry->set_id(id);
     MutableEntry meta_entry(session->write_transaction(),
-                            syncable::GET_BY_ID,
-                            id);
+                            syncable::GET_BY_ID, id);
     CHECK(meta_entry.good());
-    // This is the only change we make to the entry in this function.
-    meta_entry.Put(syncable::SYNCING, true);
 
     DCHECK(0 != session->routing_info().count(meta_entry.GetModelType()))
         << "Committing change to datatype that's not actively enabled.";
diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index ae6b8ae..2f39c23 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -19,6 +19,35 @@ namespace browser_sync {
 using sessions::SyncSession;
 using sessions::StatusController;
 
+namespace {
+
+// Sets the SYNCING bits of all items in the commit set to value_to_set.
+void SetAllSyncingBitsToValue(WriteTransaction* trans,
+                              const sessions::OrderedCommitSet& commit_set,
+                              bool value_to_set) {
+  const std::vector<syncable::Id>& commit_ids = commit_set.GetAllCommitIds();
+  for (std::vector<syncable::Id>::const_iterator it = commit_ids.begin();
+       it != commit_ids.end(); ++it) {
+    syncable::MutableEntry entry(trans, syncable::GET_BY_ID, *it);
+    if (entry.good()) {
+      entry.Put(syncable::SYNCING, value_to_set);
+    }
+  }
+}
+
+// Sets the SYNCING bits for all items in the OrderedCommitSet.
+void SetSyncingBits(WriteTransaction* trans,
+                    const sessions::OrderedCommitSet& commit_set) {
+  SetAllSyncingBitsToValue(trans, commit_set, true);
+}
+
+// Clears the SYNCING bits for all items in the OrderedCommitSet.
+void ClearSyncingBits(syncable::Directory* dir,
+                      const sessions::OrderedCommitSet& commit_set) {
+  WriteTransaction trans(FROM_HERE, SYNCER, dir);
+  SetAllSyncingBitsToValue(&trans, commit_set, false);
+}
+
 // Helper function that finds sync items that are ready to be committed to the
 // server and serializes them into a commit message protobuf.  It will return
 // false iff there are no entries ready to be committed at this time.
@@ -48,53 +77,77 @@ bool PrepareCommitMessage(sessions::SyncSession* session,
   get_commit_ids_command.Execute(session);
 
   DVLOG(1) << "Commit message will contain " << commit_set->Size() << " items.";
-  if (commit_set->Empty())
+  if (commit_set->Empty()) {
     return false;
+  }
 
   // Serialize the message.
   BuildCommitCommand build_commit_command(*commit_set, commit_message);
   build_commit_command.Execute(session);
 
+  SetSyncingBits(session->write_transaction(), *commit_set);
   return true;
 }
 
-SyncerError BuildAndPostCommits(Syncer* syncer,
-                                sessions::SyncSession* session) {
-  StatusController* status_controller = session->mutable_status_controller();
-
-  sessions::OrderedCommitSet commit_set(session->routing_info());
+SyncerError BuildAndPostCommitsImpl(Syncer* syncer,
+                                    sessions::SyncSession* session,
+                                    sessions::OrderedCommitSet* commit_set) {
   ClientToServerMessage commit_message;
-  while (PrepareCommitMessage(session, &commit_set, &commit_message)
-         && !syncer->ExitRequested()) {
+  while (!syncer->ExitRequested() &&
+         PrepareCommitMessage(session, commit_set, &commit_message)) {
     ClientToServerResponse commit_response;
 
     DVLOG(1) << "Sending commit message.";
     TRACE_EVENT_BEGIN0("sync", "PostCommit");
-    status_controller->set_last_post_commit_result(
-        SyncerProtoUtil::PostClientToServerMessage(commit_message,
-                                                   &commit_response,
-                                                   session));
+    const SyncerError post_result = SyncerProtoUtil::PostClientToServerMessage(
+        commit_message, &commit_response, session);
     TRACE_EVENT_END0("sync", "PostCommit");
 
-    // ProcessCommitResponse includes some code that cleans up after a failure
-    // to post a commit message, so we must run it regardless of whether or not
-    // the commit succeeds.
+    if (post_result != SYNCER_OK) {
+      LOG(WARNING) << "Post commit failed";
+      return post_result;
+    }
+
+    if (!commit_response.has_commit()) {
+      LOG(WARNING) << "Commit response has no commit body!";
+      return SERVER_RESPONSE_VALIDATION_FAILED;
+    }
+
+    const size_t num_responses = commit_response.commit().entryresponse_size();
+    if (num_responses != commit_set->Size()) {
+      LOG(ERROR)
+         << "Commit response has wrong number of entries! "
+         << "Expected: " << commit_set->Size() << ", "
+         << "Got: " << num_responses;
+      return SERVER_RESPONSE_VALIDATION_FAILED;
+    }
 
     TRACE_EVENT_BEGIN0("sync", "ProcessCommitResponse");
     ProcessCommitResponseCommand process_response_command(
-        commit_set, commit_message, commit_response);
-    status_controller->set_last_process_commit_response_result(
-            process_response_command.Execute(session));
+        *commit_set, commit_message, commit_response);
+    const SyncerError processing_result =
+        process_response_command.Execute(session);
     TRACE_EVENT_END0("sync", "ProcessCommitResponse");
 
-    // Exit early if either the commit or the response processing failed.
-    if (status_controller->last_post_commit_result() != SYNCER_OK)
-      return status_controller->last_post_commit_result();
-    if (status_controller->last_process_commit_response_result() != SYNCER_OK)
-      return status_controller->last_process_commit_response_result();
+    if (processing_result != SYNCER_OK) {
+      return processing_result;
+    }
   }
 
   return SYNCER_OK;
 }
 
+}  // namespace
+
+
+SyncerError BuildAndPostCommits(Syncer* syncer,
+                                sessions::SyncSession* session) {
+  sessions::OrderedCommitSet commit_set(session->routing_info());
+  SyncerError result = BuildAndPostCommitsImpl(syncer, session, &commit_set);
+  if (result != SYNCER_OK) {
+    ClearSyncingBits(session->context()->directory(), commit_set);
+  }
+  return result;
+}
+
 }  // namespace browser_sync
diff --git a/sync/engine/model_changing_syncer_command.cc b/sync/engine/model_changing_syncer_command.cc
index 9409746..f6da0f3 100644
--- a/sync/engine/model_changing_syncer_command.cc
+++ b/sync/engine/model_changing_syncer_command.cc
@@ -15,10 +15,7 @@ namespace browser_sync {
 SyncerError ModelChangingSyncerCommand::ExecuteImpl(
     sessions::SyncSession* session) {
   work_session_ = session;
-  SyncerError result = ModelNeutralExecuteImpl(work_session_);
-
-  if (result != SYNCER_OK)
-    return result;
+  SyncerError result = SYNCER_OK;
 
   const std::set<ModelSafeGroup>& groups_to_change =
       GetGroupsToChange(*work_session_);
@@ -51,9 +48,4 @@ SyncerError ModelChangingSyncerCommand::ExecuteImpl(
   return result;
 }
 
-SyncerError ModelChangingSyncerCommand::ModelNeutralExecuteImpl(
-    sessions::SyncSession* session) {
-  return SYNCER_OK;
-}
-
 }  // namespace browser_sync
diff --git a/sync/engine/model_changing_syncer_command.h b/sync/engine/model_changing_syncer_command.h
index e2e8956..fba8d11 100644
--- a/sync/engine/model_changing_syncer_command.h
+++ b/sync/engine/model_changing_syncer_command.h
@@ -55,14 +55,6 @@ class ModelChangingSyncerCommand : public SyncerCommand {
   virtual std::set<ModelSafeGroup> GetGroupsToChange(
       const sessions::SyncSession& session) const = 0;
 
-  // Sometimes, a command has work to do that needs to touch global state
-  // belonging to multiple ModelSafeGroups, but in a way that is known to be
-  // safe.  This will be called once, prior to ModelChangingExecuteImpl,
-  // *without* a ModelSafeGroup restriction in place on the SyncSession.
-  // Returns true on success, false on failure.
-  // TODO(tim): Remove this (bug 36594).
-  virtual SyncerError ModelNeutralExecuteImpl(sessions::SyncSession* session);
-
   // Abstract method to be implemented by subclasses to handle logic that
   // operates on the model.  This is invoked with a SyncSession ModelSafeGroup
   // restriction in place so that bits of state belonging to data types
diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc
index 6514bc1..dfecd39 100644
--- a/sync/engine/process_commit_response_command.cc
+++ b/sync/engine/process_commit_response_command.cc
@@ -74,33 +74,6 @@ std::set<ModelSafeGroup> ProcessCommitResponseCommand::GetGroupsToChange(
   return groups_with_commits;
 }
 
-SyncerError ProcessCommitResponseCommand::ModelNeutralExecuteImpl(
-    SyncSession* session) {
-  syncable::Directory* dir = session->context()->directory();
-  const vector<syncable::Id>& commit_ids = commit_set_.GetAllCommitIds();
-
-  if (!commit_response_.has_commit()) {
-    LOG(WARNING) << "Commit response has no commit body!";
-    ClearSyncingBits(dir, commit_ids);
-    return SERVER_RESPONSE_VALIDATION_FAILED;
-  }
-
-  const CommitResponse& cr = commit_response_.commit();
-  int commit_count = commit_set_.Size();
-  if (cr.entryresponse_size() != commit_count) {
-    LOG(ERROR) << "Commit response has wrong number of entries! Expected:" <<
-               commit_count << " Got:" << cr.entryresponse_size();
-    for (int i = 0 ; i < cr.entryresponse_size() ; i++) {
-      LOG(ERROR) << "Response #" << i << " Value: " <<
-                 cr.entryresponse(i).response_type();
-      if (cr.entryresponse(i).has_error_message())
-        LOG(ERROR) << "  " << cr.entryresponse(i).error_message();
-    }
-    ClearSyncingBits(dir, commit_ids);
-    return SERVER_RESPONSE_VALIDATION_FAILED;
-  }
-  return SYNCER_OK;
-}
 
 SyncerError ProcessCommitResponseCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
@@ -500,22 +473,4 @@ void ProcessCommitResponseCommand::ProcessSuccessfulCommitResponse(
   }
 }
 
-void ProcessCommitResponseCommand::ClearSyncingBits(
-    syncable::Directory *dir,
-    const vector<syncable::Id>& commit_ids) {
-  // This is part of the cleanup in the case of a failed commit.  Normally we
-  // would unset the SYNCING bit when processing the commit response.  In the
-  // failure case we don't process the response, so we need to clear those bits
-  // here.
-  syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
-  for (size_t i = 0; i < commit_ids.size(); i++) {
-    syncable::MutableEntry entry(&trans, syncable::GET_BY_ID, commit_ids[i]);
-    if (entry.good()) {
-      entry.Put(syncable::SYNCING, false);
-    } else {
-      LOG(WARNING) << "Id: " << commit_ids[i] << " disappeared";
-    }
-  }
-}
-
 }  // namespace browser_sync
diff --git a/sync/engine/process_commit_response_command.h b/sync/engine/process_commit_response_command.h
index 0680cc4..b17809f 100644
--- a/sync/engine/process_commit_response_command.h
+++ b/sync/engine/process_commit_response_command.h
@@ -61,8 +61,6 @@ class ProcessCommitResponseCommand : public ModelChangingSyncerCommand {
   // ModelChangingSyncerCommand implementation.
   virtual std::set<ModelSafeGroup> GetGroupsToChange(
       const sessions::SyncSession& session) const OVERRIDE;
-  virtual SyncerError ModelNeutralExecuteImpl(
-      sessions::SyncSession* session) OVERRIDE;
   virtual SyncerError ModelChangingExecuteImpl(
       sessions::SyncSession* session) OVERRIDE;
 
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index f8639c0..1a299db 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -182,7 +182,8 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         break;
       }
       case COMMIT: {
-        BuildAndPostCommits(this, session);
+        session->mutable_status_controller()->set_commit_result(
+            BuildAndPostCommits(this, session));
         next_step = RESOLVE_CONFLICTS;
         break;
       }
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 65742eb3..1cd9753 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -792,7 +792,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersUnreadyEntries) {
   {
     const StatusController& status_controller = session_->status_controller();
     // Expect success.
-    EXPECT_EQ(status_controller.last_post_commit_result(), SYNCER_OK);
+    EXPECT_EQ(status_controller.error().commit_result, SYNCER_OK);
     // None should be unsynced anymore.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 21, 21, ids_, &rtrans);
@@ -953,7 +953,7 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) {
   EXPECT_EQ(2, status().syncer_status().num_server_overwrites);
   EXPECT_EQ(1, status().syncer_status().num_local_overwrites);
   // We successfully commited item(s).
-  EXPECT_EQ(status().last_post_commit_result(), SYNCER_OK);
+  EXPECT_EQ(status().error().commit_result, SYNCER_OK);
   SyncShareNudge();
 
   // Everything should be resolved now. The local changes should have
@@ -961,7 +961,7 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) {
   // overwrote the local for entry 3.
   EXPECT_EQ(0, status().syncer_status().num_server_overwrites);
   EXPECT_EQ(0, status().syncer_status().num_local_overwrites);
-  EXPECT_EQ(status().last_post_commit_result(), SYNCER_OK);
+  EXPECT_EQ(status().error().commit_result, SYNCER_OK);
   ReadTransaction rtrans(FROM_HERE, directory());
   VERIFY_ENTRY(1, false, false, false, 0, 41, 41, ids_, &rtrans);
   VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
@@ -2728,7 +2728,7 @@ TEST_F(SyncerTest, CommitManyItemsInOneGo_PostBufferFail) {
   EXPECT_EQ(1U, mock_server_->commit_messages().size());
   EXPECT_FALSE(session_->Succeeded());
   EXPECT_EQ(SYNC_SERVER_ERROR,
-            session_->status_controller().error().last_post_commit_result);
+            session_->status_controller().error().commit_result);
   EXPECT_EQ(items_to_commit - kDefaultMaxCommitBatchSize,
             directory()->unsynced_entity_count());
 }
diff --git a/sync/internal_api/public/sessions/error_counters.cc b/sync/internal_api/public/sessions/error_counters.cc
index 72ed2d1..1982e2d 100644
--- a/sync/internal_api/public/sessions/error_counters.cc
+++ b/sync/internal_api/public/sessions/error_counters.cc
@@ -9,8 +9,7 @@ namespace sessions {
 
 ErrorCounters::ErrorCounters()
     : last_download_updates_result(UNSET),
-      last_post_commit_result(UNSET),
-      last_process_commit_response_result(UNSET) {
+      commit_result(UNSET) {
 }
 
 }  // namespace sessions
diff --git a/sync/internal_api/public/sessions/error_counters.h b/sync/internal_api/public/sessions/error_counters.h
index a2f528c..e513c84 100644
--- a/sync/internal_api/public/sessions/error_counters.h
+++ b/sync/internal_api/public/sessions/error_counters.h
@@ -23,8 +23,7 @@ struct ErrorCounters {
 
   // Records the most recent results of PostCommit and GetUpdates commands.
   SyncerError last_download_updates_result;
-  SyncerError last_post_commit_result;
-  SyncerError last_process_commit_response_result;
+  SyncerError commit_result;
 };
 
 }  // namespace sessions
diff --git a/sync/internal_api/public/sessions/sync_session_snapshot.h b/sync/internal_api/public/sessions/sync_session_snapshot.h
index 9ed9465..45f6c75 100644
--- a/sync/internal_api/public/sessions/sync_session_snapshot.h
+++ b/sync/internal_api/public/sessions/sync_session_snapshot.h
@@ -68,7 +68,6 @@ class SyncSessionSnapshot {
   int num_hierarchy_conflicts() const;
   int num_simple_conflicts() const;
   int num_server_conflicts() const;
-  bool did_commit_items() const;
   SyncSourceInfo source() const;
   bool notifications_enabled() const;
   size_t num_entries() const;
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 8f6c9ee..0e8e6a6 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -169,21 +169,8 @@ void StatusController::set_last_download_updates_result(
   shared_.error.mutate()->last_download_updates_result = result;
 }
 
-void StatusController::set_last_post_commit_result(const SyncerError result) {
-  shared_.error.mutate()->last_post_commit_result = result;
-}
-
-SyncerError StatusController::last_post_commit_result() const {
-  return shared_.error.value().last_post_commit_result;
-}
-
-void StatusController::set_last_process_commit_response_result(
-    const SyncerError result) {
-  shared_.error.mutate()->last_process_commit_response_result = result;
-}
-
-SyncerError StatusController::last_process_commit_response_result() const {
-  return shared_.error.value().last_process_commit_response_result;
+void StatusController::set_commit_result(const SyncerError result) {
+  shared_.error.mutate()->commit_result = result;
 }
 
 void StatusController::update_conflicts_resolved(bool resolved) {
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index d91efec..ebb77ba 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -157,9 +157,6 @@ class StatusController {
     return ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS);
   }
 
-  SyncerError last_post_commit_result() const;
-  SyncerError last_process_commit_response_result() const;
-
   // A toolbelt full of methods for updating counters and flags.
   void set_num_server_changes_remaining(int64 changes_remaining);
   void set_invalid_store(bool invalid_store);
@@ -174,8 +171,7 @@ class StatusController {
   void increment_num_server_overwrites();
   void set_sync_protocol_error(const SyncProtocolError& error);
   void set_last_download_updates_result(const SyncerError result);
-  void set_last_post_commit_result(const SyncerError result);
-  void set_last_process_commit_response_result(const SyncerError result);
+  void set_commit_result(const SyncerError result);
 
   void update_conflicts_resolved(bool resolved);
   void reset_conflicts_resolved();
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index 9f38e80..2e0f44d 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -66,12 +66,8 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.set_last_download_updates_result(SYNCER_OK);
   EXPECT_EQ(SYNCER_OK, status.error().last_download_updates_result);
 
-  status.set_last_post_commit_result(SYNC_AUTH_ERROR);
-  EXPECT_EQ(SYNC_AUTH_ERROR, status.error().last_post_commit_result);
-
-  status.set_last_process_commit_response_result(SYNC_SERVER_ERROR);
-  EXPECT_EQ(SYNC_SERVER_ERROR,
-            status.error().last_process_commit_response_result);
+  status.set_commit_result(SYNC_AUTH_ERROR);
+  EXPECT_EQ(SYNC_AUTH_ERROR, status.error().commit_result);
 
   for (int i = 0; i < 14; i++)
     status.increment_num_successful_commits();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index ac874d1..36bc2ca 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -235,12 +235,8 @@ bool IsError(SyncerError error) {
 bool HadErrors(const ErrorCounters& error) {
   const bool download_updates_error =
       IsError(error.last_download_updates_result);
-  const bool post_commit_error = IsError(error.last_post_commit_result);
-  const bool process_commit_response_error =
-      IsError(error.last_process_commit_response_result);
-  return download_updates_error ||
-         post_commit_error ||
-         process_commit_response_error;
+  const bool commit_error = IsError(error.commit_result);
+  return download_updates_error || commit_error;
 }
 }  // namespace
 
@@ -251,9 +247,7 @@ bool SyncSession::Succeeded() const {
 
 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;
+  bool reached_server = error.last_download_updates_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
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 701072e..a54c8f00 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -22,7 +22,7 @@ void SimulateDownloadUpdatesFailed(sessions::SyncSession* session,
 
 void SimulateCommitFailed(sessions::SyncSession* session,
                           SyncerStep begin, SyncerStep end) {
-  session->mutable_status_controller()->set_last_post_commit_result(
+  session->mutable_status_controller()->set_commit_result(
       SERVER_RETURN_TRANSIENT_ERROR);
 }
 
@@ -42,10 +42,7 @@ void SimulateSuccess(sessions::SyncSession* session,
   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);
+    session->mutable_status_controller()->set_commit_result(SYNCER_OK);
   }
 }