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

chrome/browser/sync/profile_sync_service_unittest.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stack>
 #include <vector>
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "base/file_util.h"
 #include "base/message_loop.h"
@@ skipping 36 matching lines @@
 using browser_sync::SyncBackendHost;
 using browser_sync::SyncBackendHostMock;
 using browser_sync::UnrecoverableErrorHandler;
 using testing::_;
 using testing::Return;
 using testing::WithArg;
 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.

+    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;
   }
-  virtual bool GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id) {
-    return helper_->GetSyncIdForTaggedNode(this, tag, sync_id);
-  }
+
  private:
-  scoped_ptr<TestModelAssociatorHelper> helper_;
+  browser_sync::TestIdFactory* id_factory_;
 };
 
 // FakeServerChange constructs a list of sync_api::ChangeRecords while modifying
 // the sync model, and can pass the ChangeRecord list to a
 // sync_api::SyncObserver (i.e., the ProfileSyncService) to test the client
 // change-application behavior.
 // Tests using FakeServerChange should be careful to avoid back-references,
 // since FakeServerChange will send the edits in the order specified.
 class FakeServerChange {
  public:
@@ skipping 258 matching lines @@
                                                          &gnext));
       EXPECT_EQ(gnode.GetSuccessorId(), gnext.GetId());
       EXPECT_EQ(gnode.GetParentId(), gnext.GetParentId());
     }
     if (bnode->GetChildCount()) {
       EXPECT_TRUE(gnode.GetFirstChildId());
     }
   }
 
   void ExpectSyncerNodeMatching(const BookmarkNode* bnode) {
-    sync_api::ReadTransaction trans(service_->backend_->GetUserShareHandle());
+    sync_api::ReadTransaction trans(service_->GetUserShareHandle());
     ExpectSyncerNodeMatching(&trans, bnode);
   }
 
   void ExpectBrowserNodeMatching(sync_api::BaseTransaction* trans,
                                  int64 sync_id) {
     EXPECT_TRUE(sync_id);
     const BookmarkNode* bnode =
         associator()->GetChromeNodeFromSyncId(sync_id);
     ASSERT_TRUE(bnode);
     int64 id = associator()->GetSyncIdFromChromeId(bnode->id());
@@ skipping 57 matching lines @@
       ExpectBrowserNodeMatching(trans, id);
 
       sync_api::ReadNode gnode(trans);
       ASSERT_TRUE(gnode.InitByIdLookup(id));
       stack.push(gnode.GetFirstChildId());
       stack.push(gnode.GetSuccessorId());
     }
   }
 
   void ExpectModelMatch() {
-    sync_api::ReadTransaction trans(service_->backend_->GetUserShareHandle());
+    sync_api::ReadTransaction trans(service_->GetUserShareHandle());
     ExpectModelMatch(&trans);
   }
 
   int64 other_bookmarks_id() {
     return associator()->GetSyncIdFromChromeId(model_->other_node()->id());
   }
 
   int64 bookmark_bar_id() {
     return associator()->GetSyncIdFromChromeId(
         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,
   // though -- OnInitializationCompleted is the only example (so far) in this
   // test where we need to Run the loop to swallow a task and then quit, to
   // avoid leaking the ProfileSyncService (the PostTask will retain the callee
   // and caller until the task is run).
   MessageLoop message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
@@ skipping 93 matching lines @@
   // Delete an item with several children.
   model_->Remove(folder2->GetParent(),
                  folder2->GetParent()->IndexOfChild(folder2));
   ExpectModelMatch();
 }
 
 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);
   int64 f2 = adds.AddFolder(L"Server Folder A", bookmark_bar_id(), f1);
   int64 u1 = adds.AddURL(L"Some old site", "ftp://nifty.andrew.cmu.edu/",
                          bookmark_bar_id(), f2);
   int64 u2 = adds.AddURL(L"Nifty", "ftp://nifty.andrew.cmu.edu/", f1, 0);
   // u3 is a duplicate URL
   int64 u3 = adds.AddURL(L"Nifty2", "ftp://nifty.andrew.cmu.edu/", f1, u2);
   // u4 is a duplicate title, different URL.
@@ skipping 68 matching lines @@
 }
 
 // Tests a specific case in ApplyModelChanges where we move the
 // children out from under a parent, and then delete the parent
 // in the same changelist.  The delete shows up first in the changelist,
 // requiring the children to be moved to a temporary location.
 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.
   std::string url("http://dev.chromium.org/");
   FakeServerChange adds(&trans);
   int64 f0 = other_bookmarks_id();                 // + other_node
   int64 f1 = adds.AddFolder(L"f1",      f0, 0);    //   + f1
   int64 f2 = adds.AddFolder(L"f2",      f1, 0);    //     + f2
   int64 u3 = adds.AddURL(   L"u3", url, f2, 0);    //       + u3    NOLINT
   int64 u4 = adds.AddURL(   L"u4", url, f2, u3);   //       + u4    NOLINT
@@ skipping 28 matching lines @@
 
   ExpectModelMatch(&trans);
 }
 
 // Simulate a server change record containing a valid but non-canonical URL.
 TEST_F(ProfileSyncServiceTest, ServerChangeWithNonCanonicalURL) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, SAVE_TO_STORAGE);
   StartSyncService();
 
   {
-    sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+    sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
     FakeServerChange adds(&trans);
     std::string url("http://dev.chromium.org");
     EXPECT_NE(GURL(url).spec(), url);
     adds.AddURL(L"u1", url, other_bookmarks_id(), 0);
 
     adds.ApplyPendingChanges(change_processor());
 
     EXPECT_TRUE(model_->other_node()->GetChildCount() == 1);
     ExpectModelMatch(&trans);
@@ skipping 10 matching lines @@
 }
 
 // Simulate a server change record containing an invalid URL (per GURL).
 // TODO(ncarter): Disabled due to crashes.  Fix bug 1677563.
 TEST_F(ProfileSyncServiceTest, DISABLED_ServerChangeWithInvalidURL) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, SAVE_TO_STORAGE);
   StartSyncService();
 
   int child_count = 0;
   {
-    sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
+    sync_api::WriteTransaction trans(service_->GetUserShareHandle());
 
     FakeServerChange adds(&trans);
     std::string url("x");
     EXPECT_FALSE(GURL(url).is_valid());
     adds.AddURL(L"u1", url, other_bookmarks_id(), 0);
 
     adds.ApplyPendingChanges(change_processor());
 
     // We're lenient about what should happen -- the model could wind up with
     // the node or without it; but things should be consistent, and we
@@ skipping 96 matching lines @@
 
   // Add a node which will be the target of the consistency violation.
   const BookmarkNode* node =
       model_->AddGroup(model_->other_node(), 0, ASCIIToUTF16("node"));
   ExpectSyncerNodeMatching(node);
 
   // Now destroy the syncer node as if we were the ProfileSyncService without
   // 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));
     sync_node.Remove();
   }
   // The models don't match at this point, but the ProfileSyncService
   // doesn't know it yet.
   ExpectSyncerNodeKnown(node);
   EXPECT_TRUE(service_->ShouldPushChanges());
 
@@ skipping 468 matching lines @@
 
 // See what happens if we enable sync but then delete the "Sync Data"
 // folder.
 TEST_F(ProfileSyncServiceTestWithData, RecoverAfterDeletingSyncDataDirectory) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, SAVE_TO_STORAGE);
   StartSyncService();
 
   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
   // directory).
   StopSyncService(SAVE_TO_STORAGE);
 
   // Now pretend that the user has deleted this directory from the disk.
   file_util::Delete(sync_data_directory, true);
 
   // Restart the sync service.  Don't fake out setting initial sync ended; lets
@@ skipping 44 matching lines @@
 
     service_->RegisterDataTypeController(
         new browser_sync::BookmarkDataTypeController(&factory_,
                                                      profile_.get(),
                                                      service_.get()));
 
     service_->Initialize();  // will call disableForUser because sync setup
                              // hasn't been completed.
   }
 
-  ASSERT_FALSE(service_->backend());
   ASSERT_FALSE(service_->HasSyncSetupCompleted());
 
   // Create some tokens in the token service; the service will startup when
   // it is notified that tokens are available.
   profile_->GetTokenService()->IssueAuthTokenForTest(
       GaiaConstants::kSyncService, "sync_token");
 
   syncable::ModelTypeSet set;
   set.insert(syncable::BOOKMARKS);
   service_->OnUserChoseDatatypes(false, set);
 
   MessageLoop::current()->RunAllPending();
 
   // Stop the service so we can read the new Sync Data files that were created.
   service_.reset();
 
   // This file should have been deleted when the whole directory was nuked.
   ASSERT_FALSE(file_util::PathExists(sync_file3));
   ASSERT_FALSE(file_util::PathExists(sync_file1));
 
   // This will still exist, but the text should have changed.
   ASSERT_TRUE(file_util::PathExists(sync_file2));
   std::string file2text;
   file_util::ReadFileToString(sync_file2, &file2text);
   ASSERT_FALSE(file2text.compare(nonsense2) == 0);
 }