[Sync::USS] Add bookmarks merge logic on first enable sync

This CL add the logic to merge the local model with the remote model
upon first enable sync.


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

chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc

@@ -113,17 +113,6 @@ class SingleClientBookmarksSyncTestIncludingUssTests
 IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestIncludingUssTests, Sanity) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
-  // TODO(crbug.com/516866): Move the following block after adding the bookmark
-  // nodes to the model upon implementing the merge logic in USS. This is here
-  // to make sure that when sync starts, the model is completely empty and
-  // doesn't require any merge logic which isn't the general case obviously.
-
-  // Setup sync, wait for its completion, and make sure changes were synced.
-  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  ASSERT_TRUE(
-      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
-  ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
-
   // Starting state:
   // other_node
   //    -> top
@@ -152,8 +141,11 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestIncludingUssTests, Sanity) {
       kSingleProfileIndex, tier1_b, 0, "tier1_b_url0",
       GURL("http://www.nhl.com"));
 
-  // TODO(crbug.com/516866): Call SetupSync() here upon implementing the merge
-  // logic in USS. Refer the TODO above for details.
+  // Setup sync, wait for its completion, and make sure changes were synced.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+  ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
 
   //  Ultimately we want to end up with the following model; but this test is
   //  more about the journey than the destination.

components/sync_bookmarks/BUILD.gn

@@ -14,6 +14,8 @@ static_library("sync_bookmarks") {
     "bookmark_local_changes_builder.h",
     "bookmark_model_associator.cc",
     "bookmark_model_associator.h",
+    "bookmark_model_merger.cc",
+    "bookmark_model_merger.h",
     "bookmark_model_observer_impl.cc",
     "bookmark_model_observer_impl.h",
     "bookmark_model_type_processor.cc",
@@ -45,6 +47,7 @@ source_set("unit_tests") {
 
   sources = [
     "bookmark_data_type_controller_unittest.cc",
+    "bookmark_model_merger_unittest.cc",
     "bookmark_model_observer_impl_unittest.cc",
     "bookmark_model_type_processor_unittest.cc",
     "bookmark_remote_updates_handler_unittest.cc",

components/sync_bookmarks/bookmark_model_merger.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_MERGER_H_
+#define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_MERGER_H_
+
+#include <unordered_map>
+#include <vector>
+
+#include "base/macros.h"
+#include "components/sync/engine/non_blocking_sync_common.h"
+
+namespace bookmarks {
+class BookmarkModel;
+class BookmarkNode;
+}  // namespace bookmarks
+
+namespace sync_bookmarks {
+
+class SyncedBookmarkTracker;
+
+// Responsible for merging local and remote bookmark models when bookmark sync
+// is enabled for the first time by the user (i.e. no sync metadata exists
+// locally), so we need a best-effort merge based on similarity. It implements
+// similar logic to that in BookmarkModelAssociator::AssociateModels() to be
+// used by the BookmarkModelTypeProcessor().
+class BookmarkModelMerger {
+ public:
+  // |updates|, |bookmark_model| and |bookmark_tracker| must not be null and
+  // must outlive this object.
+  BookmarkModelMerger(const syncer::UpdateResponseDataList* updates,
+                      bookmarks::BookmarkModel* bookmark_model,
+                      SyncedBookmarkTracker* bookmark_tracker);
+
+  ~BookmarkModelMerger();
+
+  // Merges the remote bookmark model represented as the |updates| received from
+  // the sync server and local bookmark model |bookmark_model|, and updates the
+  // model and |bookmark_tracker| (all of which are injected in the constructor)
+  // accordingly. On return, there will be a 1:1 mapping between bookmark nodes
+  // and metadata entities in the injected tracker.
+  void Merge();
+
+ private:
+  // Merges a local and a remote subtrees. The input nodes are two equivalent
+  // local and remote nodes. This method tries to recursively match their
+  // children. It updates the |bookmark_tracker_| accordingly.
+  void MergeSubtree(const bookmarks::BookmarkNode* local_node,
+                    const syncer::UpdateResponseData* remote_update);
+
+  // Creates a local bookmark node for a |remote_update|. The local node is
+  // created under |local_parent| at position |index|. If they the remote node
+  // has children, this method recursively creates them as well. It updates the
+  // |bookmark_tracker_| accordingly.
+  void ProcessRemoteCreation(const syncer::UpdateResponseData* remote_update,
+                             const bookmarks::BookmarkNode* local_parent,
+                             int index);
+
+  // Creates a server counter-part for the local node at position |index|
+  // under |parent|. If the local node has children, corresponding server nodes
+  // are created recursively. It updates the |bookmark_tracker_| accordingly and
+  // new nodes are marked to be committed.
+  void ProcessLocalCreation(const bookmarks::BookmarkNode* parent, int index);
+
+  // Gets the bookmark node corresponding to a permanent folder.
+  // |update_entity| must contain server_defined_unique_tag that is used to
+  // determine the corresponding permanent node.
+  const bookmarks::BookmarkNode* GetPermanentFolder(
+      const syncer::EntityData& update_entity) const;
+
+  const syncer::UpdateResponseDataList* const updates_;
+  bookmarks::BookmarkModel* const bookmark_model_;
+  SyncedBookmarkTracker* const bookmark_tracker_;
+  // Stores the tree of |updates_| as a map from a remote node to a
+  // vector of remote children. It's constructed in the c'tor.
+  const std::unordered_map<const syncer::UpdateResponseData*,
+                           std::vector<const syncer::UpdateResponseData*>>
+      updates_tree_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkModelMerger);
+};
+
+}  // namespace sync_bookmarks
+
+#endif  // COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_MERGER_H_

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -17,6 +17,7 @@
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync/protocol/bookmark_model_metadata.pb.h"
 #include "components/sync_bookmarks/bookmark_local_changes_builder.h"
+#include "components/sync_bookmarks/bookmark_model_merger.h"
 #include "components/sync_bookmarks/bookmark_model_observer_impl.h"
 #include "components/sync_bookmarks/bookmark_remote_updates_handler.h"
 #include "components/undo/bookmark_undo_utils.h"
@@ -135,10 +136,20 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
   DCHECK(model_type_state.initial_sync_done());
 
   if (!bookmark_tracker_) {
-    // TODO(crbug.com/516866): Implement the merge logic.
     StartTrackingMetadata(
         std::vector<NodeMetadataPair>(),
         std::make_unique<sync_pb::ModelTypeState>(model_type_state));
+    {
+      ScopedRemoteUpdateBookmarks update_bookmarks(
+          bookmark_model_, bookmark_undo_service_,
+          bookmark_model_observer_.get());
+
+      BookmarkModelMerger(&updates, bookmark_model_, bookmark_tracker_.get())
+          .Merge();
+    }
+    schedule_save_closure_.Run();
+    NudgeForCommitIfNeeded();
+    return;
   }
   // TODO(crbug.com/516866): Set the model type state.
 

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -20,29 +20,11 @@ namespace sync_bookmarks {
 
 namespace {
 
-// The sync protocol identifies top-level entities by means of well-known tags,
-// (aka server defined tags) which should not be confused with titles or client
-// tags that aren't supported by bookmarks (at the time of writing). Each tag
-// corresponds to a singleton instance of a particular top-level node in a
-// user's share; the tags are consistent across users. The tags allow us to
-// locate the specific folders whose contents we care about synchronizing,
-// without having to do a lookup by name or path.  The tags should not be made
-// user-visible. For example, the tag "bookmark_bar" represents the permanent
-// node for bookmarks bar in Chrome. The tag "other_bookmarks" represents the
-// permanent folder Other Bookmarks in Chrome.
-//
-// It is the responsibility of something upstream (at time of writing, the sync
-// server) to create these tagged nodes when initializing sync for the first
-// time for a user.  Thus, once the backend finishes initializing, the
-// ProfileSyncService can rely on the presence of tagged nodes.
-const char kBookmarkBarTag[] = "bookmark_bar";
-const char kMobileBookmarksTag[] = "synced_bookmarks";
-const char kOtherBookmarksTag[] = "other_bookmarks";
-
 // 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
 // emit updates in top-down order. |ordered_updates| must not be null because
@@ -214,19 +196,20 @@ BookmarkRemoteUpdatesHandler::ReorderUpdates(
 
 void BookmarkRemoteUpdatesHandler::ProcessRemoteCreate(
     const syncer::UpdateResponseData& update) {
-  // Because the Synced Bookmarks node can be created server side, it's possible
-  // it'll arrive at the client as an update. In that case it won't have been
-  // associated at startup, the GetChromeNodeFromSyncId call above will return
-  // null, and we won't detect it as a permanent node, resulting in us trying to
-  // create it here (which will fail). Therefore, we add special logic here just
-  // to detect the Synced Bookmarks folder.
   const syncer::EntityData& update_entity = update.entity.value();
   DCHECK(!update_entity.is_deleted());
+  // Because the Synced Bookmarks node can be created server side, it's possible
+  // it'll arrive at the client as an update.
+  if (update_entity.server_defined_unique_tag == kMobileBookmarksTag) {
+    bookmark_tracker_->Add(update_entity.id, bookmark_model_->mobile_node(),
+                           update.response_version, update_entity.creation_time,
+                           update_entity.unique_position,
+                           update_entity.specifics);
+    return;
+  }
   if (update_entity.parent_id == kBookmarksRootId) {
-    // Associate permanent folders.
-    // TODO(crbug.com/516866): Method documentation says this method should be
-    // used in initial sync only. Make sure this is the case.
-    AssociatePermanentFolder(update);
+    DLOG(ERROR) << "Permanent nodes other than the Synced Bookmarks node "
+                   "should have been merged during intial sync.";
     return;
   }
   if (!IsValidBookmarkSpecifics(update_entity.specifics.bookmark(),
@@ -393,26 +376,4 @@ const bookmarks::BookmarkNode* BookmarkRemoteUpdatesHandler::GetParentNode(
   return parent_entity->bookmark_node();
 }
 
-void BookmarkRemoteUpdatesHandler::AssociatePermanentFolder(
-    const syncer::UpdateResponseData& update) {
-  const syncer::EntityData& update_entity = update.entity.value();
-  DCHECK_EQ(update_entity.parent_id, kBookmarksRootId);
-
-  const bookmarks::BookmarkNode* permanent_node = nullptr;
-  if (update_entity.server_defined_unique_tag == kBookmarkBarTag) {
-    permanent_node = bookmark_model_->bookmark_bar_node();
-  } else if (update_entity.server_defined_unique_tag == kOtherBookmarksTag) {
-    permanent_node = bookmark_model_->other_node();
-  } else if (update_entity.server_defined_unique_tag == kMobileBookmarksTag) {
-    permanent_node = bookmark_model_->mobile_node();
-  }
-
-  if (permanent_node != nullptr) {
-    bookmark_tracker_->Add(update_entity.id, permanent_node,
-                           update.response_version, update_entity.creation_time,
-                           update_entity.unique_position,
-                           update_entity.specifics);
-  }
-}
-
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -77,14 +77,6 @@ class BookmarkRemoteUpdatesHandler {
   // from |bookmark_tracker_|.
   void RemoveEntityAndChildrenFromTracker(const bookmarks::BookmarkNode* node);
 
-  // Associates the permanent bookmark folders with the corresponding server
-  // side ids and registers the association in |bookmark_tracker_|.
-  // |update_entity| must contain server_defined_unique_tag that is used to
-  // determine the corresponding permanent node. All permanent nodes are assumed
-  // to be directly children nodes of |kBookmarksRootId|. This method is used in
-  // the initial sync cycle only.
-  void AssociatePermanentFolder(const syncer::UpdateResponseData& update);
-
   bookmarks::BookmarkModel* const bookmark_model_;
   SyncedBookmarkTracker* const bookmark_tracker_;
 

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -16,6 +16,7 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/protocol/unique_position.pb.h"
+#include "components/sync_bookmarks/bookmark_model_merger.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -198,6 +199,12 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
       bookmarks::TestBookmarkClient::CreateModel();
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreateBookmarkBarNodeUpdateData()};
+  // TODO(crbug.com/516866): Create a test fixture that would encapsulate
+  // the merge functionality for all relevant tests.
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(), &tracker)
+      .Merge();
 
   const std::string kId0 = "id0";
   const std::string kId1 = "id1";
@@ -205,7 +212,6 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
 
   // Constuct the updates list to have creations randomly ordered.
   syncer::UpdateResponseDataList updates;
-  updates.push_back(CreateBookmarkBarNodeUpdateData());
   updates.push_back(CreateUpdateResponseData(/*server_id=*/kId2,
                                              /*parent_id=*/kId1,
                                              /*is_deletion=*/false));
@@ -307,6 +313,10 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
       bookmarks::TestBookmarkClient::CreateModel();
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
+  const syncer::UpdateResponseDataList bookmark_bar_updates = {
+      CreateBookmarkBarNodeUpdateData()};
+  BookmarkModelMerger(&bookmark_bar_updates, bookmark_model.get(), &tracker)
+      .Merge();
 
   const std::string kId0 = "id0";
   const std::string kId1 = "id1";

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -344,6 +344,10 @@ void SyncedBookmarkTracker::UpdateUponCommitResponse(
   }
 }
 
+bool SyncedBookmarkTracker::IsEmpty() const {
+  return sync_id_to_entities_map_.empty();
+}
+
 std::size_t SyncedBookmarkTracker::TrackedEntitiesCountForTest() const {
   return sync_id_to_entities_map_.size();
 }

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -146,6 +146,9 @@ class SyncedBookmarkTracker {
                                 int64_t acked_sequence_number,
                                 int64_t server_version);
 
+  // Whether the tracker is empty or not.
+  bool IsEmpty() const;
+
   // Returns number of tracked entities. Used only in test.
   std::size_t TrackedEntitiesCountForTest() const;