sync: Loop committing items without downloading updates

Since its inception sync has required all commits to be preceded by a
GetUpdates request. This was done to try to ensure we detect and resolve
conflicts on the client, rather than having two conflicting commits
collide at the server. It could never work perfectly, but it was likely
to work in most cases and the server would bounce the commit if it
didn't.

Now we have a new server that doesn't always give us the latest version
of a node when we ask for it, especially when the request was not
solicited by the server (ie. not triggered by notification). The update
before commit is now much less likely to detect conflicts. Even worse,
the useless update requests can be surprisingly expensive in certain
scenarios.

This change allows us to avoid fetching updates between 'batches' of
commits.  This should improve performance in the case where we have lots
of items to commit (ie. first time sync) and reduce load on the server.

This CL has some far-reaching effects. This is in part because I decided
to refactor the commit code, but major changes would have been required
with or without the refactor. Highlights include:
- The commit-related SyncerCommands are now paramaterized with local
  variables, allowing us to remove many members from the SyncSession
  classes.
- Several syncer states have been collapsed into one COMMIT state
  which redirects to the new BuildAndPostCommits() function.
- The PostCommitMessageCommand had been reduced to one line, which
  has now been inlined into the BuildAndPostCommits() function.
- The unsynced_handles counter has been removed for now. Following this
  change, it would have always been zero unless an error was encountered
  during the commit. The functions that reference it expect different
  behaviour and would have been broken by this change.
- The code to put extensions activity data back into the
  ExtensionsActivityMonitor in case of failure has been moved around to
  avoid double-counting if we have to post many commit messages.
- A CONFLICT response from the server is now treated as a transient
  error.  If we had continued to treat it as a success we might risk
  going into an infinite loop. See comment in code for details.
- The "purposeless anachronism" conflicting_new_folder_ids_ has been
  removed.

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

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

1 files changed, 5 insertions, 55 deletions

diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 5de4ae5..ac6b5eb7 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -14,11 +14,10 @@
 #include "sync/engine/build_commit_command.h"
 #include "sync/engine/cleanup_disabled_types_command.h"
 #include "sync/engine/clear_data_command.h"
+#include "sync/engine/commit.h"
 #include "sync/engine/conflict_resolver.h"
 #include "sync/engine/download_updates_command.h"
-#include "sync/engine/get_commit_ids_command.h"
 #include "sync/engine/net/server_connection_manager.h"
-#include "sync/engine/post_commit_message_command.h"
 #include "sync/engine/process_commit_response_command.h"
 #include "sync/engine/process_updates_command.h"
 #include "sync/engine/resolve_conflicts_command.h"
@@ -65,9 +64,7 @@ const char* SyncerStepToString(const SyncerStep step)
     ENUM_CASE(PROCESS_UPDATES);
     ENUM_CASE(STORE_TIMESTAMPS);
     ENUM_CASE(APPLY_UPDATES);
-    ENUM_CASE(BUILD_COMMIT_REQUEST);
-    ENUM_CASE(POST_COMMIT_MESSAGE);
-    ENUM_CASE(PROCESS_COMMIT_RESPONSE);
+    ENUM_CASE(COMMIT);
     ENUM_CASE(RESOLVE_CONFLICTS);
     ENUM_CASE(APPLY_UPDATES_TO_RESOLVE_CONFLICTS);
     ENUM_CASE(CLEAR_PRIVATE_DATA);
@@ -180,59 +177,12 @@ void Syncer::SyncShare(sessions::SyncSession* session,
           last_step = SYNCER_END;
           next_step = SYNCER_END;
         } else {
-          next_step = BUILD_COMMIT_REQUEST;
+          next_step = COMMIT;
         }
         break;
       }
-      // These two steps are combined since they are executed within the same
-      // write transaction.
-      case BUILD_COMMIT_REQUEST: {
-        syncable::Directory* dir = session->context()->directory();
-        WriteTransaction trans(FROM_HERE, SYNCER, dir);
-        sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans);
-
-        DVLOG(1) << "Getting the Commit IDs";
-        GetCommitIdsCommand get_commit_ids_command(
-            session->context()->max_commit_batch_size());
-        get_commit_ids_command.Execute(session);
-
-        if (!session->status_controller().commit_ids().empty()) {
-          DVLOG(1) << "Building a commit message";
-
-          // This isn't perfect, since the set of extensions activity may not
-          // correlate exactly with the items being committed.  That's OK as
-          // long as we're looking for a rough estimate of extensions activity,
-          // not an precise mapping of which commits were triggered by which
-          // extension.
-          //
-          // We will push this list of extensions activity back into the
-          // ExtensionsActivityMonitor if this commit turns out to not contain
-          // any bookmarks, or if the commit fails.
-          session->context()->extensions_monitor()->GetAndClearRecords(
-              session->mutable_extensions_activity());
-
-          BuildCommitCommand build_commit_command;
-          build_commit_command.Execute(session);
-
-          next_step = POST_COMMIT_MESSAGE;
-        } else {
-          next_step = RESOLVE_CONFLICTS;
-        }
-
-        break;
-      }
-      case POST_COMMIT_MESSAGE: {
-        PostCommitMessageCommand post_commit_command;
-        session->mutable_status_controller()->set_last_post_commit_result(
-            post_commit_command.Execute(session));
-        next_step = PROCESS_COMMIT_RESPONSE;
-        break;
-      }
-      case PROCESS_COMMIT_RESPONSE: {
-        ProcessCommitResponseCommand process_response_command;
-        session->mutable_status_controller()->
-            set_last_process_commit_response_result(
-                process_response_command.Execute(session));
+      case COMMIT: {
+        BuildAndPostCommits(this, session);
         next_step = RESOLVE_CONFLICTS;
         break;
       }