Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(138)

Issue 2743713005: [sync] use std::multimap for BookmarkNodeMap (Closed)

Created:
3 years, 9 months ago by Patrick Noland
Modified:
3 years, 9 months ago
Reviewers:
skym
CC:
chromium-reviews, sync-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[sync] use std::multimap for BookmarkNodeMap When associating bookmarks, sync nodes are matched to BookmarkNodes. To match a BookmarkNode with a sync node, a BookmarkNodeFinder is created and asked to find the child node with the matching title. When searching for the title, BookmarkNodeFinder returns the first matching child in its child_nodes_ map. This map is an unordered_map with undefined iteration order. If sync node siblings share a title, the BookmarkNodeFinder can erroneously swap their respective BookmarkNodes. To fix this, this CL makes the iteration order defined with std::multimap, which stores pairs in insertion order. This is safe since sync nodes are processed in the same order as BookmarkNodes are stored. R=skym@chromium.org BUG=700992 Review-Url: https://codereview.chromium.org/2743713005 Cr-Commit-Position: refs/heads/master@{#456234} Committed: https://chromium.googlesource.com/chromium/src/+/e4a5c1d83e0288ebe94f1d7bae7a76d8c01efffe

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M components/sync_bookmarks/bookmark_model_associator.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14 (7 generated)
Patrick Noland
Sky, PTAL
3 years, 9 months ago (2017-03-10 23:57:45 UTC) #2
skym
lgtm
3 years, 9 months ago (2017-03-11 00:03:54 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2743713005/1
3 years, 9 months ago (2017-03-11 00:35:00 UTC) #7
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e4a5c1d83e0288ebe94f1d7bae7a76d8c01efffe
3 years, 9 months ago (2017-03-11 00:40:13 UTC) #10
Nicolas Zea
On 2017/03/11 00:40:13, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1) as ...
3 years, 9 months ago (2017-03-11 00:56:36 UTC) #11
Patrick Noland
On 2017/03/11 00:56:36, Nicolas Zea wrote: > On 2017/03/11 00:40:13, commit-bot: I haz the power ...
3 years, 9 months ago (2017-03-11 01:07:15 UTC) #12
chromium-reviews
3 years, 9 months ago (2017-03-11 01:31:38 UTC) #13
Message was sent while issue was closed.
I dunno how edge this is. Sigh out and sign in to do full merge without
sync ids. Merging this change seems very safe.

On Fri, Mar 10, 2017 at 5:07 PM <pnoland@chromium.org> wrote:

> On 2017/03/11 00:56:36, Nicolas Zea wrote:
> > On 2017/03/11 00:40:13, commit-bot: I haz the power wrote:
> > > Committed patchset #1 (id:1) as
> > >
> >
>
>
https://chromium.googlesource.com/chromium/src/+/e4a5c1d83e0288ebe94f1d7bae7a...
> >
> > Nice find! That said, it would have been good to file a bug for this,
> which
> > would make it easier to merge. As it is, would still be good to merge the
> change
> > if we think it will reduce bookmark duplication.
>
> It would reduce bookmark duplication in the wild, but it's a bit of an edge
> case. You need to have duplicated titles within the same folder and be
> doing a
> full association. How urgent does it need to be to justify a merge?
>
> https://codereview.chromium.org/2743713005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698