Use external ID to match native model nodes during bookmark association.

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

Index: chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
index 3f8f10350df1878975ca62c642975acbc2a23098..b678b6e43bfc3f6204eb901063d83483bb2cd832 100644
--- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
@@ -43,7 +43,7 @@
 #include "sync/internal_api/public/write_node.h"
 #include "sync/internal_api/public/write_transaction.h"
 #include "sync/internal_api/syncapi_internal.h"
-#include "sync/syncable/mutable_entry.h"  // TODO(tim): Remove. Bug 131130.
+#include "sync/syncable/mutable_entry.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -374,16 +374,17 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
   // the sync model directly after ModelAssociation.  This function can be
   // invoked prior to model association to set up first-time sync model
   // association scenarios.
-  int64 AddBookmarkToShare(syncer::WriteTransaction *trans,
+  int64 AddBookmarkToShare(syncer::WriteTransaction* trans,
                            int64 parent_id,
-                           std::string title) {
+                           const std::string& title,
+                           const std::string& url) {
     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_url(url);
     specifics.set_title(title);
 
     syncer::WriteNode node(trans);
@@ -788,12 +789,12 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
   {
     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));
+      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));
+        AddBookmarkToShare(
+            &trans, folder_id, base::StringPrintf("bookmark%05d", j),
+            base::StringPrintf("http://www.google.com/search?q=%05d", j));
       }
     }
   }
@@ -804,6 +805,189 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
   ExpectModelMatch();
 }
 
+// Tests bookmark association when nodes exists in the native model only.
+// These entries should be copied to Sync directory during association process.
+TEST_F(ProfileSyncServiceBookmarkTest,
+       InitialModelAssociateWithBookmarkModelNodes) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const BookmarkNode* folder =
+      model_->AddFolder(model_->other_node(), 0, base::ASCIIToUTF16("foobar"));
+  model_->AddFolder(folder, 0, base::ASCIIToUTF16("nested"));
+  model_->AddURL(folder, 0, base::ASCIIToUTF16("Internets #1 Pies Site"),
+                 GURL("http://www.easypie.com/"));
+  model_->AddURL(folder, 1, base::ASCIIToUTF16("Airplanes"),
+                 GURL("http://www.easyjet.com/"));
+
+  StartSync();
+  ExpectModelMatch();
+}
+
+// Tests bookmark association case when there is an entry in the delete journal
+// that matches one of native bookmarks.
+TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociateWithDeleteJournal) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const BookmarkNode* folder = model_->AddFolder(model_->bookmark_bar_node(), 0,
+                                                 base::ASCIIToUTF16("foobar"));
+  const BookmarkNode* bookmark =
+      model_->AddURL(folder, 0, base::ASCIIToUTF16("Airplanes"),
+                     GURL("http://www.easyjet.com/"));
+
+  CreatePermanentBookmarkNodes();
+
+  // Create entries matching the folder and the bookmark above.
+  int64 folder_id, bookmark_id;
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    folder_id = AddFolderToShare(&trans, "foobar");
+    bookmark_id = AddBookmarkToShare(&trans, folder_id, "Airplanes",
+                                     "http://www.easyjet.com/");
+  }
+
+  // Associate the bookmark sync node with the native model one and make
+  // it deleted.
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(bookmark_id));
+
+    node.GetMutableEntryForTest()->PutLocalExternalId(bookmark->id());
+    node.GetMutableEntryForTest()->PutServerIsDel(true);
+    node.GetMutableEntryForTest()->PutIsDel(true);
+  }
+
+  ASSERT_TRUE(AssociateModels());
+  ExpectModelMatch();
+
+  // The bookmark node should be deleted.
+  EXPECT_EQ(0, folder->child_count());
+}
+
+// Tests that the external ID is used to match the right folder amoung
+// multiple folders with the same name during the native model traversal.
+// Also tests that the external ID is used to match the right bookmark
+// among multiple identical bookmarks when dealing with the delete journal.
+TEST_F(ProfileSyncServiceBookmarkTest,
+       InitialModelAssociateVerifyExternalIdMatch) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const int kNumFolders = 10;
+  const int kNumBookmarks = 10;
+  const int kFolderToIncludeBookmarks = 7;
+  const int kBookmarkToDelete = 4;
+
+  int64 folder_ids[kNumFolders];
+  int64 bookmark_ids[kNumBookmarks];
+
+  // Create native folders and bookmarks with identical names. Only
+  // one of the folders contains bookmarks and others are empty. Here is the
+  // expected tree shape:
+  // Bookmarks bar
+  // +- folder (#0)
+  // +- folder (#1)
+  // ...
+  // +- folder (#7) <-- Only this folder contains bookmarks.
+  //    +- bookmark (#0)
+  //    +- bookmark (#1)
+  //    ...
+  //    +- bookmark (#4) <-- Only this one bookmark should be removed later.
+  //    ...
+  //    +- bookmark (#9)
+  // ...
+  // +- folder (#9)
+
+  const BookmarkNode* parent_folder = nullptr;
+  for (int i = 0; i < kNumFolders; i++) {
+    const BookmarkNode* folder = model_->AddFolder(
+        model_->bookmark_bar_node(), i, base::ASCIIToUTF16("folder"));
+    folder_ids[i] = folder->id();
+    if (i == kFolderToIncludeBookmarks) {
+      parent_folder = folder;
+    }
+  }
+
+  for (int i = 0; i < kNumBookmarks; i++) {
+    const BookmarkNode* bookmark =
+        model_->AddURL(parent_folder, i, base::ASCIIToUTF16("bookmark"),
+                       GURL("http://www.google.com/"));
+    bookmark_ids[i] = bookmark->id();
+  }
+
+  // Number of nodes in bookmark bar before association.
+  int total_node_count = model_->bookmark_bar_node()->GetTotalNodeCount();
+
+  CreatePermanentBookmarkNodes();
+
+  int64 sync_bookmark_id_to_delete = 0;
+  {
+    // Create sync folders matching native folders above.
+    int64 parent_id = 0;
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    // Create in reverse order because AddFolderToShare passes NULL for
+    // |predecessor| argument.
+    for (int i = kNumFolders - 1; i >= 0; i--) {
+      int64 id = AddFolderToShare(&trans, "folder");
+
+      // Pre-map sync folders to native folders by setting
+      // external ID. This will verify that the association algorithm picks
+      // the right ones despite all of them having identical names.
+      // More specifically this will help to avoid cloning bookmarks from
+      // a wrong folder.
+      syncer::WriteNode node(&trans);
+      EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
+      node.GetMutableEntryForTest()->PutLocalExternalId(folder_ids[i]);
+
+      if (i == kFolderToIncludeBookmarks) {
+        parent_id = id;
+      }
+    }
+
+    // Create sync bookmark matching native bookmarks above in reverse order
+    // because AddBookmarkToShare passes NULL for |predecessor| argument.
+    for (int i = kNumBookmarks - 1; i >= 0; i--) {
+      int id = AddBookmarkToShare(&trans, parent_id, "bookmark",
+                                  "http://www.google.com/");
+
+      // Pre-map sync bookmarks to native bookmarks by setting
+      // external ID. This will verify that the association algorithm picks
+      // the right ones despite all of them having identical names and URLs.
+      syncer::WriteNode node(&trans);
+      EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
+      node.GetMutableEntryForTest()->PutLocalExternalId(bookmark_ids[i]);
+
+      if (i == kBookmarkToDelete) {
+        sync_bookmark_id_to_delete = id;
+      }
+    }
+  }
+
+  // Make one bookmark deleted.
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByIdLookup(sync_bookmark_id_to_delete));
+
+    node.GetMutableEntryForTest()->PutServerIsDel(true);
+    node.GetMutableEntryForTest()->PutIsDel(true);
+  }
+
+  // Perform association.
+  ASSERT_TRUE(AssociateModels());
+  ExpectModelMatch();
+
+  // Only one native node should have been deleted and no nodes cloned due to
+  // matching folder names.
+  EXPECT_EQ(kNumFolders, model_->bookmark_bar_node()->child_count());
+  EXPECT_EQ(kNumBookmarks - 1, parent_folder->child_count());
+  EXPECT_EQ(total_node_count - 1,
+            model_->bookmark_bar_node()->GetTotalNodeCount());
+
+  // Verify that the right bookmark got deleted and no bookmarks reordered.
+  for (int i = 0; i < parent_folder->child_count(); i++) {
+    int index_in_bookmark_ids = (i < kBookmarkToDelete) ? i : i + 1;
+    EXPECT_EQ(bookmark_ids[index_in_bookmark_ids],
+              parent_folder->GetChild(i)->id());
+  }
+}
 
 TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);