From afc8770181f528d391e74af41e9be125814e3009 Mon Sep 17 00:00:00 2001 From: Mikel Astiz Date: Mon, 25 Nov 2019 10:53:45 +0000 Subject: [PATCH] Refactor bookmark GUID-matching logic Prior to this patch, the two existing functions to build GUID-keyed maps were heavily coupled and there were fragile underlying assumptions that made it work (e.g. there were potentially entries in |guid_to_remote_node_map_| that didn't actually match any local GUID). Instead, let's centralize the matching logic into one single function, and adopt a unified map to represent the matches. This simplifies the code and enforces more strict invariants without the need for DCHECKs. Bug: 978430 Change-Id: Ieea0db8d43376e204b793dcc64b2dc84d521b289 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929216 Commit-Queue: Mikel Astiz Reviewed-by: Mohamed Amir Yosef Cr-Commit-Position: refs/heads/master@{#718580} --- .../sync_bookmarks/bookmark_model_merger.cc | 133 ++++++++---------- .../sync_bookmarks/bookmark_model_merger.h | 40 ++---- .../bookmark_specifics_conversions.cc | 6 + 3 files changed, 81 insertions(+), 98 deletions(-) diff --git a/components/sync_bookmarks/bookmark_model_merger.cc b/components/sync_bookmarks/bookmark_model_merger.cc index 861403dbe05a..dfd19a27a592 100644 --- a/components/sync_bookmarks/bookmark_model_merger.cc +++ b/components/sync_bookmarks/bookmark_model_merger.cc @@ -237,9 +237,8 @@ BookmarkModelMerger::BookmarkModelMerger( favicon_service_(favicon_service), bookmark_tracker_(bookmark_tracker), remote_forest_(BuildRemoteForest(std::move(updates))), - guid_to_remote_node_map_(BuildGUIDToRemoteNodeMap(remote_forest_)), - guid_to_local_node_map_( - BuildGUIDToLocalNodeMap(bookmark_model_, guid_to_remote_node_map_)) { + guid_to_match_map_( + FindGuidMatchesOrReassignLocal(remote_forest_, bookmark_model_)) { DCHECK(bookmark_tracker_->IsEmpty()); DCHECK(favicon_service); } @@ -307,63 +306,62 @@ BookmarkModelMerger::RemoteForest BookmarkModelMerger::BuildRemoteForest( } // static -std::unordered_map -BookmarkModelMerger::BuildGUIDToRemoteNodeMap( - const RemoteForest& remote_forest) { +std::unordered_map +BookmarkModelMerger::FindGuidMatchesOrReassignLocal( + const RemoteForest& remote_forest, + bookmarks::BookmarkModel* bookmark_model) { + DCHECK(bookmark_model); + // TODO(crbug.com/978430): Handle potential duplicate GUIDs within remote // updates. - std::unordered_map - guid_to_remote_node_map; + if (!base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) { - return guid_to_remote_node_map; + return {}; } + // Build a temporary lookup table for remote GUIDs. + std::unordered_map + guid_to_remote_node_map; for (const auto& tree_tag_and_root : remote_forest) { tree_tag_and_root.second.EmplaceSelfAndDescendantsByGUID( &guid_to_remote_node_map); } - return guid_to_remote_node_map; -} - -// static -std::unordered_map -BookmarkModelMerger::BuildGUIDToLocalNodeMap( - bookmarks::BookmarkModel* bookmark_model, - const std::unordered_map& - guid_to_remote_node_map) { - DCHECK(bookmark_model); - - std::unordered_map - guid_to_local_node_map; - if (!base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) { - return guid_to_local_node_map; - } - + // Iterate through all local bookmarks to find matches by GUID. + std::unordered_map + guid_to_match_map; ui::TreeNodeIterator iterator( bookmark_model->root_node()); while (iterator.has_next()) { - const bookmarks::BookmarkNode* node = iterator.Next(); + const bookmarks::BookmarkNode* const node = iterator.Next(); DCHECK(base::IsValidGUID(node->guid())); + if (node->is_permanent_node()) { continue; } + const auto remote_it = guid_to_remote_node_map.find(node->guid()); if (remote_it == guid_to_remote_node_map.end()) { continue; } - // If local node and its remote node match are conflicting in node type or - // URL, replace local GUID with a random GUID. - const syncer::EntityData& remote_entity = remote_it->second->entity(); + + const RemoteTreeNode* const remote_node = remote_it->second; + const syncer::EntityData& remote_entity = remote_node->entity(); if (node->is_folder() != remote_entity.is_folder || (node->is_url() && node->url() != remote_entity.specifics.bookmark().url())) { - node = - ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model); + // If local node and its remote node match are conflicting in node type or + // URL, replace local GUID with a random GUID. + // TODO(crbug.com/978430): Local GUIDs should also be reassigned if they + // match a remote originator_item_id. + ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model); + continue; } - guid_to_local_node_map.emplace(node->guid(), node); + + guid_to_match_map.emplace(node->guid(), GuidMatch{node, remote_node}); } - return guid_to_local_node_map; + + return guid_to_match_map; } void BookmarkModelMerger::MergeSubtree( @@ -422,25 +420,27 @@ const bookmarks::BookmarkNode* BookmarkModelMerger::FindMatchingLocalNode( const bookmarks::BookmarkNode* local_parent, size_t local_child_start_index) const { // Try to match child by GUID. If we can't, try to match child by semantics. - const bookmarks::BookmarkNode* matching_local_node = + const bookmarks::BookmarkNode* matching_local_node_by_guid = FindMatchingLocalNodeByGUID(remote_child); - if (!matching_local_node) { - // All local nodes up to |remote_index-1| have processed already. Look for a - // matching local node starting with the local node at position - // |local_child_start_index|. FindMatchingChildBySemanticsStartingAt() - // returns kInvalidIndex in the case where no semantics match was found or - // the semantics match found is GUID-matchable to a different node. - const size_t local_index = FindMatchingChildBySemanticsStartingAt( - /*remote_node=*/remote_child, - /*local_parent=*/local_parent, - /*starting_child_index=*/local_child_start_index); - if (local_index == kInvalidIndex) { - // If no match found, return. - return nullptr; - } - matching_local_node = local_parent->children()[local_index].get(); + if (matching_local_node_by_guid) { + return matching_local_node_by_guid; + } + + // All local nodes up to |remote_index-1| have processed already. Look for a + // matching local node starting with the local node at position + // |local_child_start_index|. FindMatchingChildBySemanticsStartingAt() + // returns kInvalidIndex in the case where no semantics match was found or + // the semantics match found is GUID-matchable to a different node. + const size_t local_index = FindMatchingChildBySemanticsStartingAt( + /*remote_node=*/remote_child, + /*local_parent=*/local_parent, + /*starting_child_index=*/local_child_start_index); + if (local_index == kInvalidIndex) { + // If no match found, return. + return nullptr; } - return matching_local_node; + + return local_parent->children()[local_index].get(); } const bookmarks::BookmarkNode* @@ -471,8 +471,6 @@ BookmarkModelMerger::UpdateBookmarkNodeFromSpecificsIncludingGUID( return local_node; } DCHECK(base::IsValidGUID(specifics.guid())); - // We do not update the GUID maps upon node replacement as per the comment - // in bookmark_model_merger.h. return ReplaceBookmarkNodeGUID(local_node, specifics.guid(), bookmark_model_); } @@ -574,7 +572,7 @@ void BookmarkModelMerger::ProcessLocalCreation( if (FindMatchingRemoteNodeByGUID(node->children()[i].get())) { continue; } - ProcessLocalCreation(node, i); + ProcessLocalCreation(/*parent=*/node, i); } } @@ -612,36 +610,25 @@ const BookmarkModelMerger::RemoteTreeNode* BookmarkModelMerger::FindMatchingRemoteNodeByGUID( const bookmarks::BookmarkNode* local_node) const { DCHECK(local_node); - // Ensure matching nodes are of the same type and have the same URL, - // guaranteed by BuildGUIDToLocalNodeMap(). - const auto it = guid_to_remote_node_map_.find(local_node->guid()); - if (it == guid_to_remote_node_map_.end()) { + + const auto it = guid_to_match_map_.find(local_node->guid()); + if (it == guid_to_match_map_.end()) { return nullptr; } - const RemoteTreeNode* const remote_node = it->second; - DCHECK(remote_node); - DCHECK_EQ(local_node->is_folder(), remote_node->entity().is_folder); - DCHECK_EQ(local_node->url(), - remote_node->entity().specifics.bookmark().url()); - return remote_node; + return it->second.remote_node; } const bookmarks::BookmarkNode* BookmarkModelMerger::FindMatchingLocalNodeByGUID( const RemoteTreeNode& remote_node) const { const syncer::EntityData& remote_entity = remote_node.entity(); const auto it = - guid_to_local_node_map_.find(remote_entity.specifics.bookmark().guid()); - if (it == guid_to_local_node_map_.end()) { + guid_to_match_map_.find(remote_entity.specifics.bookmark().guid()); + if (it == guid_to_match_map_.end()) { return nullptr; } - DCHECK(!remote_entity.specifics.bookmark().guid().empty()); - const bookmarks::BookmarkNode* local_node = it->second; - // Ensure matching nodes are of the same type and have the same URL, - // guaranteed by BuildGUIDToLocalNodeMap(). - DCHECK_EQ(local_node->is_folder(), remote_entity.is_folder); - DCHECK_EQ(local_node->url(), remote_entity.specifics.bookmark().url()); - return local_node; + + return it->second.local_node; } } // namespace sync_bookmarks diff --git a/components/sync_bookmarks/bookmark_model_merger.h b/components/sync_bookmarks/bookmark_model_merger.h index 1a40fad2a074..18744920a1a4 100644 --- a/components/sync_bookmarks/bookmark_model_merger.h +++ b/components/sync_bookmarks/bookmark_model_merger.h @@ -56,6 +56,14 @@ class BookmarkModelMerger { // a permanent node, keyed by server-defined unique tag of the root. using RemoteForest = std::unordered_map; + // Represents a pair of bookmarks, one local and one remote, that have been + // matched by GUID. They are guaranteed to have the same type and URL (if + // applicable). + struct GuidMatch { + const bookmarks::BookmarkNode* local_node; + const RemoteTreeNode* remote_node; + }; + // Constructs the remote bookmark tree to be merged. Each entry in the // returned map is a permanent node, identified (keyed) by the server-defined // tag. All invalid updates are filtered out, including invalid bookmark @@ -63,22 +71,12 @@ class BookmarkModelMerger { // sends tombstones as part of the initial download. static RemoteForest BuildRemoteForest(syncer::UpdateResponseDataList updates); - // Constructs a map from GUID to corresponding remote nodes, used in the merge - // process to determine GUID-based node matches. - static std::unordered_map - BuildGUIDToRemoteNodeMap(const RemoteForest& remote_forest); - - // Constructs a map from GUID to corresponding local node, used in the merge - // process to determine GUID-based node matches. |bookmark_model| must not be - // null. |guid_to_remote_node_map| is used to detect conflicting GUID matches - // between local and remote bookmarks for the cases where they cannot be - // merged (e.g. folder vs non-folder) and in such cases regenerate a random - // GUID for the local bookmark. - static std::unordered_map - BuildGUIDToLocalNodeMap( - bookmarks::BookmarkModel* bookmark_model, - const std::unordered_map& - guid_to_remote_node_map); + // Computes bookmark pairs that should be matched by GUID. Local bookmark + // GUIDs may be regenerated for the case where they collide with a remote GUID + // that is not compatible (e.g. folder vs non-folder). + static std::unordered_map + FindGuidMatchesOrReassignLocal(const RemoteForest& remote_forest, + bookmarks::BookmarkModel* bookmark_model); // Merges a local and a remote subtrees. The input nodes are two equivalent // local and remote nodes. This method tries to recursively match their @@ -151,15 +149,7 @@ class BookmarkModelMerger { // Preprocessed remote nodes in the form a forest where each tree's root is a // permanent node. Computed upon construction via BuildRemoteForest(). const RemoteForest remote_forest_; - // Maps GUIDs to their respective remote nodes. Used for GUID-based node - // matching and is populated at the beginning of the merge process. - const std::unordered_map - guid_to_remote_node_map_; - // Maps GUIDs to their respective local nodes. Used for GUID-based - // node matching and is populated at the beginning of the merge process. - // Is not updated upon potential changes to the model during merge. - const std::unordered_map - guid_to_local_node_map_; + std::unordered_map guid_to_match_map_; DISALLOW_COPY_AND_ASSIGN(BookmarkModelMerger); }; diff --git a/components/sync_bookmarks/bookmark_specifics_conversions.cc b/components/sync_bookmarks/bookmark_specifics_conversions.cc index 98725dce2a61..74914f187509 100644 --- a/components/sync_bookmarks/bookmark_specifics_conversions.cc +++ b/components/sync_bookmarks/bookmark_specifics_conversions.cc @@ -255,6 +255,12 @@ const bookmarks::BookmarkNode* ReplaceBookmarkNodeGUID( } const bookmarks::BookmarkNode* new_node; DCHECK(base::IsValidGUID(guid)); + + if (node->guid() == guid) { + // Nothing to do. + return node; + } + if (node->is_folder()) { new_node = model->AddFolder(node->parent(), node->parent()->GetIndexOf(node), -- 2.24.1