[Sync] Refactored ProfileSyncService and remove its backend() function

chrome/browser/sync/profile_sync_service_unittest.cc

Index: chrome/browser/sync/profile_sync_service_unittest.cc
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index 5c85c1df38c82ab8e2d8cc141e9d33487c33cc29..4ee0913978bdb3ec50abbcbb443823f0c73fdbd4 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -54,16 +54,80 @@ using testing::Invoke;
 
 class TestBookmarkModelAssociator : public BookmarkModelAssociator {
  public:
-  TestBookmarkModelAssociator(TestProfileSyncService* service,
+  TestBookmarkModelAssociator(
+      TestProfileSyncService* service,
       UnrecoverableErrorHandler* persist_ids_error_handler)
       : BookmarkModelAssociator(service, persist_ids_error_handler),
-        helper_(new TestModelAssociatorHelper(service->id_factory())) {
-  }
+        id_factory_(service->id_factory()) {}
+
   virtual bool GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id) {

[tim (not reviewing), 2011/01/21 18:17:00]

Im not sure I get what happened in here. Are you sure it isn't the case that
every other test could and *should* use TestModelAssociatorHelper but decided to
do things themselves instead (probably 'cause they didn't know this existed)? 
If so we just took a step backwards :)

[akalin, 2011/01/21 21:21:09]

So this logic basically lazily creates any tagged node that is requested.  I
think all the other datatype unittests instead manually create sync nodes
themselves by converting from the chrome type (e.g., the autofill test has
AddAutofillSyncNode() functions), which I think is the better way to go.  If
anything, the same thing probably should be done for bookmark tests (which
should be moved out of this class) and this logic removed.

Added TODOs.

-    return helper_->GetSyncIdForTaggedNode(this, tag, sync_id);
+    std::wstring tag_wide;
+    if (!UTF8ToWide(tag.c_str(), tag.length(), &tag_wide)) {
+      NOTREACHED() << "Unable to convert UTF8 to wide for string: " << tag;
+      return false;
+    }
+
+    browser_sync::SyncBackendHost::UserShareHandle share(
+        sync_service_->GetUserShareHandle());
+    bool root_exists = false;
+    syncable::ModelType type = model_type();
+    {
+      sync_api::WriteTransaction trans(share);
+      sync_api::ReadNode uber_root(&trans);
+      uber_root.InitByRootLookup();
+
+      sync_api::ReadNode root(&trans);
+      root_exists = root.InitByTagLookup(
+          ProfileSyncServiceTestHelper::GetTagForType(type));
+    }
+
+    if (!root_exists) {
+      bool created = ProfileSyncServiceTestHelper::CreateRoot(
+          type,
+          sync_service_->GetUserShareHandle(),
+          id_factory_);
+      if (!created)
+        return false;
+    }
+
+    sync_api::WriteTransaction trans(share);
+    sync_api::ReadNode root(&trans);
+    EXPECT_TRUE(root.InitByTagLookup(
+        ProfileSyncServiceTestHelper::GetTagForType(type)));
+
+    // First, try to find a node with the title among the root's children.
+    // This will be the case if we are testing model persistence, and
+    // are reloading a sync repository created earlier in the test.
+    int64 last_child_id = sync_api::kInvalidId;
+    for (int64 id = root.GetFirstChildId(); id != sync_api::kInvalidId; /***/) {
+      sync_api::ReadNode child(&trans);
+      child.InitByIdLookup(id);
+      last_child_id = id;
+      if (tag_wide == child.GetTitle()) {
+        *sync_id = id;
+        return true;
+      }
+      id = child.GetSuccessorId();
+    }
+
+    sync_api::ReadNode predecessor_node(&trans);
+    sync_api::ReadNode* predecessor = NULL;
+    if (last_child_id != sync_api::kInvalidId) {
+      predecessor_node.InitByIdLookup(last_child_id);
+      predecessor = &predecessor_node;
+    }
+    sync_api::WriteNode node(&trans);
+    // Create new fake tagged nodes at the end of the ordering.
+    node.InitByCreation(type, root, predecessor);
+    node.SetIsFolder(true);
+    node.SetTitle(tag_wide);
+    node.SetExternalId(0);
+    *sync_id = node.GetId();
+    return true;
   }
+
  private:
-  scoped_ptr<TestModelAssociatorHelper> helper_;
+  browser_sync::TestIdFactory* id_factory_;
 };
 
 // FakeServerChange constructs a list of sync_api::ChangeRecords while modifying
@@ -342,7 +406,7 @@ class ProfileSyncServiceTest : public testing::Test {
   }
 
   void ExpectSyncerNodeMatching(const BookmarkNode* bnode) {
-    sync_api::ReadTransaction trans(service_->backend_->GetUserShareHandle());
+    sync_api::ReadTransaction trans(service_->GetUserShareHandle());
     ExpectSyncerNodeMatching(&trans, bnode);
   }
 
@@ -420,7 +484,7 @@ class ProfileSyncServiceTest : public testing::Test {
   }
 
   void ExpectModelMatch() {
-    sync_api::ReadTransaction trans(service_->backend_->GetUserShareHandle());
+    sync_api::ReadTransaction trans(service_->GetUserShareHandle());
     ExpectModelMatch(&trans);
   }
 
@@ -433,8 +497,6 @@ class ProfileSyncServiceTest : public testing::Test {
         model_->GetBookmarkBarNode()->id());
   }
 
-  SyncBackendHost* backend() { return service_->backend_.get(); }
-
   // This serves as the "UI loop" on which the ProfileSyncService lives and
   // operates. It is needed because the SyncBackend can post tasks back to
   // the service, meaning it can't be null. It doesn't have to be running,
@@ -548,7 +610,7 @@ TEST_F(ProfileSyncServiceTest, ServerChangeProcessing) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
   StartSyncService();
 
-  sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+  sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
   FakeServerChange adds(&trans);
   int64 f1 = adds.AddFolder(L"Server Folder B", bookmark_bar_id(), 0);
@@ -637,7 +699,7 @@ TEST_F(ProfileSyncServiceTest, ServerChangeRequiringFosterParent) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
   StartSyncService();
 
-  sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+  sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
   // Stress the immediate children of other_node because that's where
   // ApplyModelChanges puts a temporary foster parent node.
@@ -686,7 +748,7 @@ TEST_F(ProfileSyncServiceTest, ServerChangeWithNonCanonicalURL) {
   StartSyncService();
 
   {
-    sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+    sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
     FakeServerChange adds(&trans);
     std::string url("http://dev.chromium.org");
@@ -717,7 +779,7 @@ TEST_F(ProfileSyncServiceTest, DISABLED_ServerChangeWithInvalidURL) {
 
   int child_count = 0;
   {
-    sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+    sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
     FakeServerChange adds(&trans);
     std::string url("x");
@@ -834,7 +896,7 @@ TEST_F(ProfileSyncServiceTest, UnrecoverableErrorSuspendsService) {
   // updating the ProfileSyncService state.  This should introduce
   // inconsistency between the two models.
   {
-    sync_api::WriteTransaction trans(service_->backend_->GetUserShareHandle());
+    sync_api::WriteTransaction trans(service_->GetUserShareHandle());
     sync_api::WriteNode sync_node(&trans);
     EXPECT_TRUE(associator()->InitSyncNodeFromChromeId(node->id(),
                                                        &sync_node));
@@ -1323,7 +1385,8 @@ TEST_F(ProfileSyncServiceTestWithData, RecoverAfterDeletingSyncDataDirectory) {
   WriteTestDataToBookmarkModel();
 
   // While the service is running.
-  FilePath sync_data_directory = backend()->sync_data_folder_path();
+  FilePath sync_data_directory =
+      service_->GetTestBackend()->sync_data_folder_path();
 
   // Simulate a normal shutdown for the sync service (don't disable it for
   // the user, which would reset the preferences and delete the sync data
@@ -1388,7 +1451,6 @@ TEST_F(ProfileSyncServiceTestWithData, TestStartupWithOldSyncData) {
                              // hasn't been completed.
   }
 
-  ASSERT_FALSE(service_->backend());
   ASSERT_FALSE(service_->HasSyncSetupCompleted());
 
   // Create some tokens in the token service; the service will startup when