[Sync:USS] Fix permanent folders detection

Permanent folders such as the "bookmark bar" are placed directly under
the bookmarks root node.

Before this CL:
Permanent folders were assumed to have a specifics parent id.
This is wrong, since the bookmark root node is synced and assigned a
non-fixed id on the server. This caused dropping the mobile bookmarks
folder when received as a remote creation.

After this CL:
Permanent nodes are detected via the existence of a unique server tag.

Bug: 516866
Change-Id: I237613ace3cc037febfb3f57257b24cc36522628
Reviewed-on: https://chromium-review.googlesource.com/c/1257833Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596678}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -47,8 +47,6 @@ const char kBookmarkBarTag[] = "bookmark_bar";
 const char kMobileBookmarksTag[] = "synced_bookmarks";
 const char kOtherBookmarksTag[] = "other_bookmarks";
 
-const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
-
 // Heuristic to consider two nodes (local and remote) a match for the purpose of
 // merging. Two folders match if they have the same title, two bookmarks match
 // if they have the same title and url. A folder and a bookmark never match.
@@ -128,15 +126,9 @@ BuildUpdatesTreeWithoutTombstonesWithSortedChildren(
     if (update_entity.is_deleted()) {
       continue;
     }
-    // Permanent nodes should be handled differently. They must be added even
-    // if they don't have children associated to them because they are the roots
-    // from which the merge starts.
-    if (update_entity.parent_id == kBookmarksRootId ||
-        update_entity.parent_id == "0") {
-      // Make sure permanent node is added if it doesn't exist already.
-      updates_tree.emplace(&update, std::vector<const UpdateResponseData*>());
-      // No need to associate it with its parent (the root node). We start
-      // merging from permanent nodes.
+    // No need to associate permanent nodes with their parent (the root node).
+    // We start merging from the permanent nodes.
+    if (!update_entity.server_defined_unique_tag.empty()) {
       continue;
     }
     if (!syncer::UniquePosition::FromProto(update_entity.unique_position)

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -27,7 +27,6 @@ namespace {
 
 const char kBookmarkBarId[] = "bookmark_bar_id";
 const char kBookmarkBarTag[] = "bookmark_bar";
-const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
 
 syncer::UpdateResponseData CreateUpdateResponseData(
     const std::string& server_id,
@@ -61,7 +60,6 @@ syncer::UpdateResponseData CreateUpdateResponseData(
 syncer::UpdateResponseData CreateBookmarkBarNodeUpdateData() {
   syncer::EntityData data;
   data.id = kBookmarkBarId;
-  data.parent_id = kBookmarksRootId;
   data.server_defined_unique_tag = kBookmarkBarTag;
 
   data.specifics.mutable_bookmark();

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -31,7 +31,7 @@ namespace {
 
 const char kBookmarkBarTag[] = "bookmark_bar";
 const char kBookmarkBarId[] = "bookmark_bar_id";
-const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
+const char kBookmarksRootId[] = "root_id";
 const char kCacheGuid[] = "generated_id";
 
 struct BookmarkInfo {

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -22,10 +22,6 @@ namespace sync_bookmarks {
 
 namespace {
 
-// Id is created by concatenating the specifics field number and the server tag
-// similar to LookbackServerEntity::CreateId() that uses
-// GetSpecificsFieldNumberFromModelType() to compute the field number.
-const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
 const char kMobileBookmarksTag[] = "synced_bookmarks";
 
 // Recursive method to traverse a forest created by ReorderUpdates() to to
@@ -141,7 +137,7 @@ void BookmarkRemoteUpdatesHandler::Process(
     // Only non deletions and non premanent node should have valid specifics and
     // unique positions.
     if (!update_entity.is_deleted() &&
-        update_entity.parent_id != kBookmarksRootId) {
+        update_entity.server_defined_unique_tag.empty()) {
       if (!IsValidBookmarkSpecifics(update_entity.specifics.bookmark(),
                                     update_entity.is_folder)) {
         // Ignore updates with invalid specifics.
@@ -286,7 +282,7 @@ void BookmarkRemoteUpdatesHandler::ProcessCreate(
                            update_entity.specifics);
     return;
   }
-  if (update_entity.parent_id == kBookmarksRootId) {
+  if (!update_entity.server_defined_unique_tag.empty()) {
     DLOG(ERROR) << "Permanent nodes other than the Synced Bookmarks node "
                    "should have been merged during intial sync.";
     return;

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -25,6 +25,7 @@ using base::ASCIIToUTF16;
 using testing::_;
 using testing::ElementsAre;
 using testing::Eq;
+using testing::NotNull;
 
 namespace sync_bookmarks {
 
@@ -32,10 +33,12 @@ namespace {
 
 // The parent tag for children of the root entity. Entities with this parent are
 // referred to as top level enities.
-const char kRootParentTag[] = "0";
+const char kRootParentId[] = "0";
+const char kBookmarksRootId[] = "root_id";
 const char kBookmarkBarId[] = "bookmark_bar_id";
 const char kBookmarkBarTag[] = "bookmark_bar";
-const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
+const char kMobileBookmarksId[] = "synced_bookmarks_id";
+const char kMobileBookmarksTag[] = "synced_bookmarks";
 
 syncer::UpdateResponseData CreateUpdateResponseData(
     const std::string& server_id,
@@ -76,7 +79,7 @@ syncer::UpdateResponseData CreateUpdateResponseData(
 syncer::UpdateResponseData CreateBookmarkRootUpdateData() {
   syncer::EntityData data;
   data.id = syncer::ModelTypeToRootTag(syncer::BOOKMARKS);
-  data.parent_id = kRootParentTag;
+  data.parent_id = kRootParentId;
   data.server_defined_unique_tag =
       syncer::ModelTypeToRootTag(syncer::BOOKMARKS);
 
@@ -89,11 +92,13 @@ syncer::UpdateResponseData CreateBookmarkRootUpdateData() {
   return response_data;
 }
 
-syncer::UpdateResponseData CreateBookmarkBarNodeUpdateData() {
+syncer::UpdateResponseData CreatePermanentFolderUpdateData(
+    const std::string& id,
+    const std::string& tag) {
   syncer::EntityData data;
-  data.id = kBookmarkBarId;
-  data.parent_id = kBookmarksRootId;
-  data.server_defined_unique_tag = kBookmarkBarTag;
+  data.id = id;
+  data.parent_id = "root_id";
+  data.server_defined_unique_tag = tag;
 
   data.specifics.mutable_bookmark();
 
@@ -178,18 +183,48 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
 
   // Updates should be ordered such that parent node update comes first, and
   // deletions come last.
-  // node0 --> node1 --> node2 --> node4 --> node5 --> node6.
+  // node4 --> node5 --> node0 --> node1 --> node2 --> node6.
   // This is test is over verifying since the order requirements are
   // within subtrees only. (e.g it doesn't matter whether node1 comes before or
   // after node4). However, it's implemented this way for simplicity.
-  EXPECT_THAT(ordered_updates[0]->entity.value().id, Eq(ids[0]));
-  EXPECT_THAT(ordered_updates[1]->entity.value().id, Eq(ids[1]));
-  EXPECT_THAT(ordered_updates[2]->entity.value().id, Eq(ids[2]));
-  EXPECT_THAT(ordered_updates[3]->entity.value().id, Eq(ids[4]));
-  EXPECT_THAT(ordered_updates[4]->entity.value().id, Eq(ids[5]));
+  EXPECT_THAT(ordered_updates[0]->entity.value().id, Eq(ids[4]));
+  EXPECT_THAT(ordered_updates[1]->entity.value().id, Eq(ids[5]));
+  EXPECT_THAT(ordered_updates[2]->entity.value().id, Eq(ids[0]));
+  EXPECT_THAT(ordered_updates[3]->entity.value().id, Eq(ids[1]));
+  EXPECT_THAT(ordered_updates[4]->entity.value().id, Eq(ids[2]));
   EXPECT_THAT(ordered_updates[5]->entity.value().id, Eq(ids[6]));
 }
 
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
+     ShouldProcessMobileBookmarksNodeCreation) {
+  // Init the processor to have only the bookmark bar.
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
+  testing::NiceMock<favicon::MockFaviconService> favicon_service;
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(),
+                      &favicon_service, &tracker)
+      .Merge();
+  // Bookmark bar node should be tracked now.
+  ASSERT_THAT(tracker.TrackedEntitiesCountForTest(), Eq(1U));
+
+  // Construct the updates list to have mobile bookmarks node remote creation.
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(
+      CreatePermanentFolderUpdateData(kMobileBookmarksId, kMobileBookmarksTag));
+  BookmarkRemoteUpdatesHandler updates_handler(bookmark_model.get(),
+                                               &favicon_service, &tracker);
+  updates_handler.Process(updates);
+
+  // Both the bookmark bar and mobile bookmarks nodes should be tracked.
+  EXPECT_THAT(tracker.TrackedEntitiesCountForTest(), Eq(2U));
+  EXPECT_THAT(tracker.GetEntityForBookmarkNode(bookmark_model->mobile_node()),
+              NotNull());
+}
+
 TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
      ShouldProcessRandomlyOrderedCreations) {
   // Prepare creation updates to construct this structure:
@@ -203,7 +238,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   const syncer::UpdateResponseDataList bookmark_bar_updates = {
-      CreateBookmarkBarNodeUpdateData()};
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
   // TODO(crbug.com/516866): Create a test fixture that would encapsulate
   // the merge functionality for all relevant tests.
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
@@ -321,7 +356,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   const syncer::UpdateResponseDataList bookmark_bar_updates = {
-      CreateBookmarkBarNodeUpdateData()};
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
   BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(),
                       &favicon_service, &tracker)
@@ -340,7 +375,8 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
 
   // Constuct the updates list to have creations randomly ordered.
   syncer::UpdateResponseDataList updates;
-  updates.push_back(CreateBookmarkBarNodeUpdateData());
+  updates.push_back(
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag));
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/kId2, /*parent_id=*/kBookmarkBarId,
       /*is_deletion=*/false, /*unique_position=*/pos2));
@@ -580,7 +616,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   const syncer::UpdateResponseDataList bookmark_bar_updates = {
-      CreateBookmarkBarNodeUpdateData()};
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
   // TODO(crbug.com/516866): Create a test fixture that would encapsulate
   // the merge functionality for all relevant tests.
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
@@ -632,7 +668,7 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   const syncer::UpdateResponseDataList bookmark_bar_updates = {
-      CreateBookmarkBarNodeUpdateData()};
+      CreatePermanentFolderUpdateData(kBookmarkBarId, kBookmarkBarTag)};
   // TODO(crbug.com/516866): Create a test fixture that would encapsulate
   // the merge functionality for all relevant tests.
   testing::NiceMock<favicon::MockFaviconService> favicon_service;