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..fd77bfa3214ac97c134fbf4754ee6f6fd86d6753 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,168 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
   ExpectModelMatch();
 }
 
+TEST_F(ProfileSyncServiceBookmarkTest,

[Nicolas Zea, 2015/02/06 23:34:19]

nit: Could you add comments to these tests describing what they're testing?
(this wasn't enforced well previously, but it's a good habit to have)

[stanisc, 2015/02/09 19:15:37]

Done.

+       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();
+}
+
+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 mode should be deleted.

[Nicolas Zea, 2015/02/06 23:34:19]

nit: mode -> node

[stanisc, 2015/02/09 19:15:37]

Done.

+  EXPECT_EQ(0, folder->child_count());
+}
+
+// Tests that AssociateModels uses external ID match when associating and
+// deleting bookmark model nodes.
+TEST_F(ProfileSyncServiceBookmarkTest,
+       InitialModelAssociateVerifyExternalIdMatch) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const int kNumFolders = 10;

[Nicolas Zea, 2015/02/06 23:34:19]

Comment describing what the expected directory structure is like? (i.e. 10
bookmark folders, with the 7th containing 10 url bookmarks...etc.

[stanisc, 2015/02/09 19:15:37]

Done.

+  const int kNumBookmarks = 10;
+  const int kFolderToIncludeBookmarks = 7;
+  const int kBookmarkToDelete = 4;
+
+  const BookmarkNode* parent_folder = nullptr;
+  int64 folder_ids[kNumFolders];
+  int64 bookmark_ids[kNumBookmarks];
+
+  // Add native folders - all with the same name.
+  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;
+    }
+  }
+
+  // Add native bookmarks - all identical.
+  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);