diff options
Diffstat (limited to '0001-Refactor-bookmark-GUID-matching-logic.patch')
| -rw-r--r-- | 0001-Refactor-bookmark-GUID-matching-logic.patch | 329 |
1 files changed, 0 insertions, 329 deletions
diff --git a/0001-Refactor-bookmark-GUID-matching-logic.patch b/0001-Refactor-bookmark-GUID-matching-logic.patch deleted file mode 100644 index 3c0a732efc6e..000000000000 --- a/0001-Refactor-bookmark-GUID-matching-logic.patch +++ /dev/null @@ -1,329 +0,0 @@ -From afc8770181f528d391e74af41e9be125814e3009 Mon Sep 17 00:00:00 2001 -From: Mikel Astiz <mastiz@chromium.org> -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 <mastiz@chromium.org> -Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> -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<std::string, const BookmarkModelMerger::RemoteTreeNode*> --BookmarkModelMerger::BuildGUIDToRemoteNodeMap( -- const RemoteForest& remote_forest) { -+std::unordered_map<std::string, BookmarkModelMerger::GuidMatch> -+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<std::string, const RemoteTreeNode*> -- 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<std::string, const RemoteTreeNode*> -+ 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<std::string, const bookmarks::BookmarkNode*> --BookmarkModelMerger::BuildGUIDToLocalNodeMap( -- bookmarks::BookmarkModel* bookmark_model, -- const std::unordered_map<std::string, const RemoteTreeNode*>& -- guid_to_remote_node_map) { -- DCHECK(bookmark_model); -- -- std::unordered_map<std::string, const bookmarks::BookmarkNode*> -- 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<std::string, BookmarkModelMerger::GuidMatch> -+ guid_to_match_map; - ui::TreeNodeIterator<const bookmarks::BookmarkNode> 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<std::string, RemoteTreeNode>; - -+ // 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<std::string, const RemoteTreeNode*> -- 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<std::string, const bookmarks::BookmarkNode*> -- BuildGUIDToLocalNodeMap( -- bookmarks::BookmarkModel* bookmark_model, -- const std::unordered_map<std::string, const RemoteTreeNode*>& -- 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<std::string, GuidMatch> -+ 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<std::string, const RemoteTreeNode*> -- 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<std::string, const bookmarks::BookmarkNode*> -- guid_to_local_node_map_; -+ std::unordered_map<std::string, GuidMatch> 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 - |