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

1 files changed, 74 insertions, 5 deletions

diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 304dd5a..1bdc13a 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -105,7 +105,6 @@ using syncable::SPECIFICS;
 using syncable::SYNCING;
 using syncable::UNITTEST;
 
-using sessions::ScopedSetSessionWriteTransaction;
 using sessions::StatusController;
 using sessions::SyncSessionContext;
 using sessions::SyncSession;
@@ -409,18 +408,16 @@ class SyncerTest : public testing::Test,
                         const vector<syncable::Id>& expected_id_order) {
     for (size_t limit = expected_id_order.size() + 2; limit > 0; --limit) {
       WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
-      ScopedSetSessionWriteTransaction set_trans(session_.get(), &wtrans);
 
       ModelSafeRoutingInfo routes;
       GetModelSafeRoutingInfo(&routes);
       sessions::OrderedCommitSet output_set(routes);
-      GetCommitIdsCommand command(limit, &output_set);
+      GetCommitIdsCommand command(&wtrans, limit, &output_set);
       std::set<int64> ready_unsynced_set;
       command.FilterUnreadyEntries(&wtrans, ModelTypeSet(),
                                    ModelTypeSet(), false,
                                    unsynced_handle_view, &ready_unsynced_set);
-      command.BuildCommitIds(session_->write_transaction(), routes,
-                             ready_unsynced_set);
+      command.BuildCommitIds(&wtrans, routes, ready_unsynced_set);
       size_t truncated_size = std::min(limit, expected_id_order.size());
       ASSERT_EQ(truncated_size, output_set.Size());
       for (size_t i = 0; i < truncated_size; ++i) {
@@ -4924,4 +4921,76 @@ TEST_F(SyncerPositionTiebreakingTest, MidLowHigh) {
   ExpectLocalOrderIsByServerId();
 }
 
+enum {
+  TEST_PARAM_BOOKMARK_ENABLE_BIT,
+  TEST_PARAM_AUTOFILL_ENABLE_BIT,
+  TEST_PARAM_BIT_COUNT
+};
+
+class MixedResult :
+    public SyncerTest,
+    public ::testing::WithParamInterface<int> {
+ protected:
+  bool ShouldFailBookmarkCommit() {
+    return (GetParam() & (1 << TEST_PARAM_BOOKMARK_ENABLE_BIT)) == 0;
+  }
+  bool ShouldFailAutofillCommit() {
+    return (GetParam() & (1 << TEST_PARAM_AUTOFILL_ENABLE_BIT)) == 0;
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(ExtensionsActivity,
+                        MixedResult,
+                        testing::Range(0, 1 << TEST_PARAM_BIT_COUNT));
+
+TEST_P(MixedResult, ExtensionsActivity) {
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+
+    MutableEntry pref(&wtrans, CREATE, PREFERENCES, wtrans.root_id(), "pref");
+    ASSERT_TRUE(pref.good());
+    pref.Put(syncable::IS_UNSYNCED, true);
+
+    MutableEntry bookmark(
+        &wtrans, CREATE, BOOKMARKS, wtrans.root_id(), "bookmark");
+    ASSERT_TRUE(bookmark.good());
+    bookmark.Put(syncable::IS_UNSYNCED, true);
+
+    if (ShouldFailBookmarkCommit()) {
+      mock_server_->SetTransientErrorId(bookmark.Get(ID));
+    }
+
+    if (ShouldFailAutofillCommit()) {
+      mock_server_->SetTransientErrorId(pref.Get(ID));
+    }
+  }
+
+
+  // Put some extenions activity records into the monitor.
+  {
+    ExtensionsActivityMonitor::Records records;
+    records["ABC"].extension_id = "ABC";
+    records["ABC"].bookmark_write_count = 2049U;
+    records["xyz"].extension_id = "xyz";
+    records["xyz"].bookmark_write_count = 4U;
+    context_->extensions_monitor()->PutRecords(records);
+  }
+
+  SyncShareNudge();
+
+  ExtensionsActivityMonitor::Records final_monitor_records;
+  context_->extensions_monitor()->GetAndClearRecords(&final_monitor_records);
+  if (ShouldFailBookmarkCommit()) {
+    ASSERT_EQ(2U, final_monitor_records.size())
+        << "Should restore records after unsuccessful bookmark commit.";
+    EXPECT_EQ("ABC", final_monitor_records["ABC"].extension_id);
+    EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
+    EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
+    EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
+  } else {
+    EXPECT_TRUE(final_monitor_records.empty())
+        << "Should not restore records after successful bookmark commit.";
+  }
+}
+
 }  // namespace syncer