Remove some members from SyncSession

This is part of the effort to shrink the size of the SyncSession and
SyncSessionJob.

One of the members to be removed was the write transaction.  This was
stored in the session in order to share it among various functions that
prepare a commit message.  The same goal can be accomplished by passing
the transaction in to the constructors of the SyncerCommands.

There was one complication in this change.  The new constructor meant
that it was impossible to instantiate a BuildCommitCommand without a
transaction, which made it somewhat harder to unit test.  Making some of
its methods static made it unnecessary to instantiate a
BuildCommitCommand object in the test.

In addition to giving us better control of the scope of the transaction,
this change allows BuildCommitCommand and GetCommitIdsCommand to use a
BaseTransaction rather than a WriteTransaction.

The other member removed from SyncSession is the
ExtensionsActivityMonitor's records.  This member was used to buffer the
ExtensionsActivityMonitor's records which were pending for commit and to
re-add them to the monitor if the commit failed.  Since it is only used
within the context of a commit, it is safer and simpler to declare it on
the stack in BuildAndPostCommitsImpl().

The change should have no noticeable impact on behaviour, though it does
move some of the interactions with the ExtensionsActivityMonitor from
the UI thread to the sync thread.  This should be safe; the monitor's
internal data structures are protected with a lock.  The move also allows
us to remove some of the code from StatusController that was used to
trigger the UI-thread-specific processing.

BUG=175024


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

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

2 files changed, 38 insertions, 12 deletions

diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc
index 1718699..3fd4a32 100644
--- a/sync/test/engine/mock_connection_manager.cc
+++ b/sync/test/engine/mock_connection_manager.cc
@@ -18,6 +18,7 @@
 #include "sync/test/engine/test_id_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using std::find;
 using std::map;
 using std::string;
 using sync_pb::ClientToServerMessage;
@@ -224,6 +225,10 @@ void MockConnectionManager::SetCommitClientCommand(
   commit_client_command_.reset(command);
 }
 
+void MockConnectionManager::SetTransientErrorId(syncable::Id id) {
+  transient_error_ids_.push_back(id);
+}
+
 sync_pb::SyncEntity* MockConnectionManager::AddUpdateBookmark(
     int id, int parent_id,
     string name, int64 version,
@@ -545,6 +550,11 @@ bool MockConnectionManager::ShouldConflictThisCommit() {
   return conflict;
 }
 
+bool MockConnectionManager::ShouldTransientErrorThisId(syncable::Id id) {
+  return find(transient_error_ids_.begin(), transient_error_ids_.end(), id)
+      != transient_error_ids_.end();
+}
+
 void MockConnectionManager::ProcessCommit(
     sync_pb::ClientToServerMessage* csm,
     sync_pb::ClientToServerResponse* response_buffer) {
@@ -559,40 +569,47 @@ void MockConnectionManager::ProcessCommit(
   for (int i = 0; i < commit_message.entries_size() ; i++) {
     const sync_pb::SyncEntity& entry = commit_message.entries(i);
     CHECK(entry.has_id_string());
-    string id = entry.id_string();
+    string id_string = entry.id_string();
     ASSERT_LT(entry.name().length(), 256ul) << " name probably too long. True "
         "server name checking not implemented";
+    syncable::Id id;
     if (entry.version() == 0) {
       // Relies on our new item string id format. (string representation of a
       // negative number).
-      committed_ids_.push_back(syncable::Id::CreateFromClientString(id));
+      id = syncable::Id::CreateFromClientString(id_string);
     } else {
-      committed_ids_.push_back(syncable::Id::CreateFromServerId(id));
+      id = syncable::Id::CreateFromServerId(id_string);
     }
-    if (response_map.end() == response_map.find(id))
-      response_map[id] = commit_response->add_entryresponse();
-    sync_pb::CommitResponse_EntryResponse* er = response_map[id];
+    committed_ids_.push_back(id);
+
+    if (response_map.end() == response_map.find(id_string))
+      response_map[id_string] = commit_response->add_entryresponse();
+    sync_pb::CommitResponse_EntryResponse* er = response_map[id_string];
     if (ShouldConflictThisCommit()) {
       er->set_response_type(CommitResponse::CONFLICT);
       continue;
     }
+    if (ShouldTransientErrorThisId(id)) {
+      er->set_response_type(CommitResponse::TRANSIENT_ERROR);
+      continue;
+    }
     er->set_response_type(CommitResponse::SUCCESS);
     er->set_version(entry.version() + 1);
     if (!commit_time_rename_prepended_string_.empty()) {
       // Commit time rename sent down from the server.
       er->set_name(commit_time_rename_prepended_string_ + entry.name());
     }
-    string parent_id = entry.parent_id_string();
+    string parent_id_string = entry.parent_id_string();
     // Remap id's we've already assigned.
-    if (changed_ids.end() != changed_ids.find(parent_id)) {
-      parent_id = changed_ids[parent_id];
-      er->set_parent_id_string(parent_id);
+    if (changed_ids.end() != changed_ids.find(parent_id_string)) {
+      parent_id_string = changed_ids[parent_id_string];
+      er->set_parent_id_string(parent_id_string);
     }
     if (entry.has_version() && 0 != entry.version()) {
-      er->set_id_string(id);  // Allows verification.
+      er->set_id_string(id_string);  // Allows verification.
     } else {
       string new_id = base::StringPrintf("mock_server:%d", next_new_id_++);
-      changed_ids[id] = new_id;
+      changed_ids[id_string] = new_id;
       er->set_id_string(new_id);
     }
   }
diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h
index 0818b72..53b950bd 100644
--- a/sync/test/engine/mock_connection_manager.h
+++ b/sync/test/engine/mock_connection_manager.h
@@ -174,6 +174,8 @@ class MockConnectionManager : public ServerConnectionManager {
   void SetGUClientCommand(sync_pb::ClientCommand* command);
   void SetCommitClientCommand(sync_pb::ClientCommand* command);
 
+  void SetTransientErrorId(syncable::Id);
+
   const std::vector<syncable::Id>& committed_ids() const {
     return committed_ids_;
   }
@@ -284,6 +286,10 @@ class MockConnectionManager : public ServerConnectionManager {
   // Determine if one entry in a commit should be rejected with a conflict.
   bool ShouldConflictThisCommit();
 
+  // Determine if the given item's commit request should be refused with
+  // a TRANSIENT_ERROR response.
+  bool ShouldTransientErrorThisId(syncable::Id id);
+
   // Generate a numeric position_in_parent value.  We use a global counter
   // that only decreases; this simulates new objects always being added to the
   // front of the ordering.
@@ -315,6 +321,9 @@ class MockConnectionManager : public ServerConnectionManager {
   // All IDs that have been committed.
   std::vector<syncable::Id> committed_ids_;
 
+  // List of IDs which should return a transient error.
+  std::vector<syncable::Id> transient_error_ids_;
+
   // Control of when/if we return conflicts.
   bool conflict_all_commits_;
   int conflict_n_commits_;