sync: Test and improve bookmark performance

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

 // Copyright (c) 2012 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.
 
 // TODO(akalin): This file is basically just a unit test for
 // BookmarkChangeProcessor.  Write unit tests for
 // BookmarkModelAssociator separately.
 
 #include <map>
 #include <queue>
 #include <stack>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/string16.h"
 #include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/base_bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/sync/glue/bookmark_change_processor.h"
 #include "chrome/browser/sync/glue/bookmark_model_associator.h"
 #include "chrome/browser/sync/glue/data_type_error_handler.h"
 #include "chrome/browser/sync/glue/data_type_error_handler_mock.h"
@@ skipping 248 matching lines @@
   }
 
   virtual void SetUp() {
     test_user_share_.SetUp();
   }
 
   virtual void TearDown() {
     test_user_share_.TearDown();
   }
 
+  // Inserts a folder directly to the share.
+  // Do not use this after model association is complete.
+  int64 AddFolderToShare(syncer::WriteTransaction* trans, std::string title) {

[tim (not reviewing), 2013/05/20 18:26:20]

It seems this is actually hard-wired to add a folder directly to the
bookmark_bar.

Can't this be made to be a special case of FakeServerChange?  Why do we need
multiple ways of adding bookmarks in these tests?  

[rlarocque, 2013/05/20 20:09:54]

It is hard-coded to add folders to the bookmark bar.  I don't see that as a
problem.  

It can't use the FakeServerChange infrastructure because FakeServerChanges are
interpreted by the ChangeProcessor.  The intent of the test I've added is to see
what happens when a pre-populated sync share is associated with an empty native
model.  This requires that the directory be populated before the change
processor exists.

+    EXPECT_FALSE(model_associator_);
+
+    // Be sure to call CreatePermanentBookmarkNodes(), otherwise this will fail.
+    syncer::ReadNode bookmark_bar(trans);
+    EXPECT_EQ(BaseNode::INIT_OK, bookmark_bar.InitByTagLookup("bookmark_bar"));
+
+    syncer::WriteNode node(trans);
+    EXPECT_TRUE(node.InitBookmarkByCreation(bookmark_bar, NULL));
+    node.SetIsFolder(true);
+    node.SetTitle(ASCIIToWide(title));
+
+    return node.GetId();
+  }
+
+  // Inserts a bookmark directly to the share.
+  // Do not use this after model association is complete.
+  int64 AddBookmarkToShare(syncer::WriteTransaction *trans,
+                           int64 parent_id,
+                           std::string title) {
+    EXPECT_FALSE(model_associator_);
+
+    syncer::ReadNode parent(trans);
+    EXPECT_EQ(BaseNode::INIT_OK, parent.InitByIdLookup(parent_id));
+
+    sync_pb::BookmarkSpecifics specifics;
+    specifics.set_url("http://www.google.com/search?q=" + title);
+    specifics.set_title(title);
+
+    syncer::WriteNode node(trans);
+    EXPECT_TRUE(node.InitBookmarkByCreation(parent, NULL));
+    node.SetIsFolder(false);
+    node.SetTitle(ASCIIToWide(title));
+    node.SetBookmarkSpecifics(specifics);
+
+    return node.GetId();
+  }
+
   // Load (or re-load) the bookmark model.  |load| controls use of the
   // bookmarks file on disk.  |save| controls whether the newly loaded
   // bookmark model will write out a bookmark file as it goes.
   void LoadBookmarkModel(LoadOption load, SaveOption save) {
     bool delete_bookmarks = load == DELETE_EXISTING_STORAGE;
     profile_.CreateBookmarkModel(delete_bookmarks);
     model_ = BookmarkModelFactory::GetForProfile(&profile_);
     ui_test_utils::WaitForBookmarkModelToLoad(model_);
     // This noticeably speeds up the unit tests that request it.
     if (save == DONT_SAVE_TO_STORAGE)
@@ skipping 318 matching lines @@
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
   StartSync();
 
   EXPECT_TRUE(other_bookmarks_id());
   EXPECT_TRUE(bookmark_bar_id());
   EXPECT_TRUE(mobile_bookmarks_id());
 
   ExpectModelMatch();
 }
 
+// Populate the sync database then start model association.  Sync's bookmarks
+// should end up being copied into the native model, resulting in a successful
+// "ExpectModelMatch()".
+TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
+  const int kNumBookmarksPerFolder = 10;
+  const int kNumFolders = 10;
+
+  CreatePermanentBookmarkNodes();
+
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    for (int i = 0; i < kNumFolders; ++i) {
+      int64 folder_id = AddFolderToShare(&trans,
+                                         base::StringPrintf("folder%05d", i));
+      for (int j = 0; j < kNumBookmarksPerFolder; ++j) {
+        AddBookmarkToShare(&trans,
+                           folder_id,
+                           base::StringPrintf("bookmark%05d", j));
+      }
+    }
+  }
+
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  StartSync();
+
+  ExpectModelMatch();
+}
+
+
 TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
   StartSync();
 
   // Test addition.
   const BookmarkNode* folder =
       model_->AddFolder(model_->other_node(), 0, ASCIIToUTF16("foobar"));
   ExpectSyncerNodeMatching(folder);
   ExpectModelMatch();
   const BookmarkNode* folder2 =
@@ skipping 1190 matching lines @@
             new_versions[changed_bookmark->id()]);
   initial_versions.erase(changed_bookmark->id());
   ExpectTransactionVersionMatch(model_->bookmark_bar_node(), initial_versions);
   ExpectTransactionVersionMatch(model_->other_node(), initial_versions);
   ExpectTransactionVersionMatch(model_->mobile_node(), initial_versions);
 }
 
 }  // namespace
 
 }  // namespace browser_sync