Refactor initial merge of sync bookmarks

No behavioral changes: various minor refactorings are introduced to
improve code readability and simplify future patches.

Bug: 978430
Change-Id: Iafca6dd38e3c918b35417cd1c65b17cc37ec03d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1917361Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715403}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -160,9 +160,9 @@ BuildUpdatesTreeWithoutTombstonesWithSortedChildren(
   }
 
   // Sort all child updates.
-  for (std::pair<const UpdateResponseData* const,
-                 std::vector<const UpdateResponseData*>>& pair : updates_tree) {
-    std::sort(pair.second.begin(), pair.second.end(), UniquePositionLessThan);
+  for (auto& parent_update_and_chilren : updates_tree) {
+    std::sort(parent_update_and_chilren.second.begin(),
+              parent_update_and_chilren.second.end(), UniquePositionLessThan);
   }
   return updates_tree;
 }
@@ -231,17 +231,18 @@ BuildGUIDToLocalNodeMap(
 }  // namespace
 
 BookmarkModelMerger::BookmarkModelMerger(
-    const UpdateResponseDataList* updates,
+    UpdateResponseDataList updates,
     bookmarks::BookmarkModel* bookmark_model,
     favicon::FaviconService* favicon_service,
     SyncedBookmarkTracker* bookmark_tracker)
-    : updates_(updates),
+    : original_updates_(std::move(updates)),
       bookmark_model_(bookmark_model),
       favicon_service_(favicon_service),
       bookmark_tracker_(bookmark_tracker),
-      updates_tree_(
-          BuildUpdatesTreeWithoutTombstonesWithSortedChildren(updates_)),
-      guid_to_remote_update_map_(BuildGUIDToRemoteUpdateMap(updates_)),
+      updates_tree_(BuildUpdatesTreeWithoutTombstonesWithSortedChildren(
+          &original_updates_)),
+      guid_to_remote_update_map_(
+          BuildGUIDToRemoteUpdateMap(&original_updates_)),
       guid_to_local_node_map_(
           BuildGUIDToLocalNodeMap(bookmark_model_,
                                   guid_to_remote_update_map_)) {
@@ -271,7 +272,7 @@ void BookmarkModelMerger::Merge() {
   // to perform the primary match. If there are multiple match candidates it
   // selects the first one.
   // Associate permanent folders.
-  for (const std::unique_ptr<UpdateResponseData>& update : *updates_) {
+  for (const std::unique_ptr<UpdateResponseData>& update : original_updates_) {
     DCHECK(update);
     const EntityData& update_entity = *update->entity;
     const bookmarks::BookmarkNode* permanent_folder =

components/sync_bookmarks/bookmark_model_merger.h

@@ -32,9 +32,9 @@ class SyncedBookmarkTracker;
 // used by the BookmarkModelTypeProcessor().
 class BookmarkModelMerger {
  public:
-  // |updates|, |bookmark_model|, |favicon_service| and |bookmark_tracker| must
-  // not be null and must outlive this object.
-  BookmarkModelMerger(const syncer::UpdateResponseDataList* updates,
+  // |bookmark_model|, |favicon_service| and |bookmark_tracker| must not be
+  // null and must outlive this object.
+  BookmarkModelMerger(syncer::UpdateResponseDataList updates,
                       bookmarks::BookmarkModel* bookmark_model,
                       favicon::FaviconService* favicon_service,
                       SyncedBookmarkTracker* bookmark_tracker);
@@ -116,7 +116,11 @@ class BookmarkModelMerger {
       const bookmarks::BookmarkNode* local_parent,
       size_t starting_child_index) const;
 
-  const syncer::UpdateResponseDataList* const updates_;
+  // Original updates as passed in the constructor, which may contain invalid
+  // updates. Needed to hold ownership of updates (other data structures such as
+  // |updates_tree_| point to these instances.
+  const syncer::UpdateResponseDataList original_updates_;
+
   bookmarks::BookmarkModel* const bookmark_model_;
   favicon::FaviconService* const favicon_service_;
   SyncedBookmarkTracker* const bookmark_tracker_;

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -128,12 +128,13 @@ bool PositionsInTrackerMatchModel(const bookmarks::BookmarkNode* node,
                      });
 }
 
-void Merge(syncer::UpdateResponseDataList* updates,
+void Merge(syncer::UpdateResponseDataList updates,
            bookmarks::BookmarkModel* bookmark_model) {
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(updates, bookmark_model, &favicon_service, &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model, &favicon_service,
+                      &tracker)
       .Merge();
 }
 
@@ -271,8 +272,8 @@ TEST(BookmarkModelMergerTest, ShouldMergeLocalAndRemoteModels) {
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&updates, bookmark_model.get(), &favicon_service,
-                      &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, &tracker)
       .Merge();
   ASSERT_THAT(bookmark_bar_node->children().size(), Eq(3u));
 
@@ -419,8 +420,8 @@ TEST(BookmarkModelMergerTest, ShouldMergeRemoteReorderToLocalModel) {
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&updates, bookmark_model.get(), &favicon_service,
-                      &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, &tracker)
       .Merge();
   ASSERT_THAT(bookmark_bar_node->children().size(), Eq(3u));
 
@@ -492,8 +493,8 @@ TEST(BookmarkModelMergerTest, ShouldMergeFaviconsForRemoteNodesOnly) {
               AddPageNoVisitForBookmark(kUrl2, base::UTF8ToUTF16(kTitle2)));
   EXPECT_CALL(favicon_service, MergeFavicon(kUrl2, _, _, _, _));
 
-  BookmarkModelMerger(&updates, bookmark_model.get(), &favicon_service,
-                      &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, &tracker)
       .Merge();
 }
 
@@ -530,8 +531,8 @@ TEST(BookmarkModelMergerTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&updates, bookmark_model.get(), &favicon_service,
-                      &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, &tracker)
       .Merge();
 
   // Both titles should have matched against each other and only node is in the
@@ -582,8 +583,8 @@ TEST(BookmarkModelMergerTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&updates, bookmark_model.get(), &favicon_service,
-                      &tracker)
+  BookmarkModelMerger(std::move(updates), bookmark_model.get(),
+                      &favicon_service, &tracker)
       .Merge();
 
   // Both titles should have matched against each other and only node is in the
@@ -609,7 +610,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeRemoteCreationWithoutGUID) {
       /*is_folder=*/true, /*unique_position=*/MakeRandomPosition(),
       /*guid=*/""));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // Node should have been created and new GUID should have been set.
   ASSERT_EQ(bookmark_model->bookmark_bar_node()->children().size(), 1u);
@@ -641,7 +642,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeAndUseRemoteGUID) {
       /*is_folder=*/true, /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kRemoteGuid));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // Node should have been replaced and GUID should be set to that stored in the
   // specifics.
@@ -675,7 +676,7 @@ TEST(BookmarkModelMergerTest,
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/""));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // Node should not have been replaced and GUID should not have been set to
   // that stored in the specifics, as it was invalid.
@@ -721,7 +722,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeBookmarkByGUID) {
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kGuid));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -776,7 +777,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeBookmarkByGUIDAndReparent) {
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kGuid));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -839,7 +840,7 @@ TEST(BookmarkModelMergerTest, ShouldMergeFolderByGUIDAndNotSemantics) {
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kGuid2));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -921,7 +922,7 @@ TEST(
       /*unique_position=*/pos2,
       /*guid=*/kGuid1));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -1005,7 +1006,7 @@ TEST(BookmarkModelMergerTest,
       /*unique_position=*/pos2,
       /*guid=*/kGuid2));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -1062,7 +1063,7 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingURLs) {
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kGuid));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar
@@ -1114,7 +1115,7 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingTypes) {
       /*unique_position=*/MakeRandomPosition(),
       /*guid=*/kGuid));
 
-  Merge(&updates, bookmark_model.get());
+  Merge(std::move(updates), bookmark_model.get());
 
   // -------- The merged model --------
   // bookmark_bar

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -242,48 +242,11 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
   DCHECK(model_type_state.initial_sync_done());
 
   if (!bookmark_tracker_) {
-    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_, favicon_service_,
-                          bookmark_tracker_.get())
-          .Merge();
-    }
-
-    // If any of the permanent nodes is missing, we treat it as failure.
-    // TODO(mamir): Revisit if this is too aggressive since it may influence
-    // the USS migrator case on desktop (which wouldn't usually have mobile
-    // bookmarks).
-    if (!bookmark_tracker_->GetEntityForBookmarkNode(
-            bookmark_model_->bookmark_bar_node()) ||
-        !bookmark_tracker_->GetEntityForBookmarkNode(
-            bookmark_model_->other_node()) ||
-        !bookmark_tracker_->GetEntityForBookmarkNode(
-            bookmark_model_->mobile_node())) {
-      LogMissingPermanentNodes(bookmark_tracker_->GetEntityForBookmarkNode(
-                                   bookmark_model_->bookmark_bar_node()),
-                               bookmark_tracker_->GetEntityForBookmarkNode(
-                                   bookmark_model_->other_node()),
-                               bookmark_tracker_->GetEntityForBookmarkNode(
-                                   bookmark_model_->mobile_node()));
-      StopTrackingMetadata();
-      bookmark_tracker_.reset();
-      error_handler_.Run(
-          syncer::ModelError(FROM_HERE, "Permanent bookmark entities missing"));
+    OnInitialUpdateReceived(model_type_state, std::move(updates));
     return;
   }
 
-    bookmark_tracker_->CheckAllNodesTracked(bookmark_model_);
-
-    schedule_save_closure_.Run();
-    NudgeForCommitIfNeeded();
-    return;
-  }
+  // Incremental updates.
   ScopedRemoteUpdateBookmarks update_bookmarks(
       bookmark_model_, bookmark_undo_service_, bookmark_model_observer_.get());
   BookmarkRemoteUpdatesHandler updates_handler(
@@ -510,6 +473,51 @@ void BookmarkModelTypeProcessor::OnBookmarkModelBeingDeleted() {
   StopTrackingMetadata();
 }
 
+void BookmarkModelTypeProcessor::OnInitialUpdateReceived(
+    const sync_pb::ModelTypeState& model_type_state,
+    syncer::UpdateResponseDataList updates) {
+  DCHECK(!bookmark_tracker_);
+
+  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(std::move(updates), bookmark_model_, favicon_service_,
+                        bookmark_tracker_.get())
+        .Merge();
+  }
+
+  // If any of the permanent nodes is missing, we treat it as failure.
+  if (!bookmark_tracker_->GetEntityForBookmarkNode(
+          bookmark_model_->bookmark_bar_node()) ||
+      !bookmark_tracker_->GetEntityForBookmarkNode(
+          bookmark_model_->other_node()) ||
+      !bookmark_tracker_->GetEntityForBookmarkNode(
+          bookmark_model_->mobile_node())) {
+    LogMissingPermanentNodes(bookmark_tracker_->GetEntityForBookmarkNode(
+                                 bookmark_model_->bookmark_bar_node()),
+                             bookmark_tracker_->GetEntityForBookmarkNode(
+                                 bookmark_model_->other_node()),
+                             bookmark_tracker_->GetEntityForBookmarkNode(
+                                 bookmark_model_->mobile_node()));
+    StopTrackingMetadata();
+    bookmark_tracker_.reset();
+    error_handler_.Run(
+        syncer::ModelError(FROM_HERE, "Permanent bookmark entities missing"));
+    return;
+  }
+
+  bookmark_tracker_->CheckAllNodesTracked(bookmark_model_);
+
+  schedule_save_closure_.Run();
+  NudgeForCommitIfNeeded();
+}
+
 void BookmarkModelTypeProcessor::StartTrackingMetadata(
     std::vector<NodeMetadataPair> nodes_metadata,
     std::unique_ptr<sync_pb::ModelTypeState> model_type_state) {

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -101,6 +101,11 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // Performs the required clean up when bookmark model is being deleted.
   void OnBookmarkModelBeingDeleted();
 
+  // Process specifically calls to OnUpdateReceived() that correspond to the
+  // initial merge of bookmarks (e.g. was just enabled).
+  void OnInitialUpdateReceived(const sync_pb::ModelTypeState& type_state,
+                               syncer::UpdateResponseDataList updates);
+
   // Instantiates the required objects to track metadata and starts observing
   // changes from the bookmark model.
   void StartTrackingMetadata(

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -168,10 +168,8 @@ class BookmarkRemoteUpdatesHandlerWithInitialMergeTest : public testing::Test {
         tracker_(std::vector<NodeMetadataPair>(),
                  std::make_unique<sync_pb::ModelTypeState>()),
         updates_handler_(bookmark_model_.get(), &favicon_service_, &tracker_) {
-    const syncer::UpdateResponseDataList permanent_folder_updates =
-        CreatePermanentFoldersUpdateData();
-    BookmarkModelMerger(&permanent_folder_updates, bookmark_model_.get(),
-                        &favicon_service_, &tracker_)
+    BookmarkModelMerger(CreatePermanentFoldersUpdateData(),
+                        bookmark_model_.get(), &favicon_service_, &tracker_)
         .Merge();
   }
 
@@ -875,10 +873,8 @@ TEST(BookmarkRemoteUpdatesHandlerTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::move(model_type_state));
 
-  const syncer::UpdateResponseDataList permanent_folder_updates =
-      CreatePermanentFoldersUpdateData();
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&permanent_folder_updates, bookmark_model.get(),
+  BookmarkModelMerger(CreatePermanentFoldersUpdateData(), bookmark_model.get(),
                       &favicon_service, &tracker)
       .Merge();
 
@@ -929,10 +925,8 @@ TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::move(model_type_state));
 
-  const syncer::UpdateResponseDataList permanent_folder_updates =
-      CreatePermanentFoldersUpdateData();
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
-  BookmarkModelMerger(&permanent_folder_updates, bookmark_model.get(),
+  BookmarkModelMerger(CreatePermanentFoldersUpdateData(), bookmark_model.get(),
                       &favicon_service, &tracker)
       .Merge();
   // Create the bookmark with same encryption key name.