1435553 - Compare synced folder timestamps to determine the base order

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler [:lina]]

I noticed this on my personal account when I added two different bookmarks on different devices to the menu. `mergeChildListsIntoMergedNode` always merges remote children first, then local. When a folder is modified on both sides, we'll move the new remote children to the *beginning* of the folder, before any local children.

This is surprising, and I think wrong. We should instead compare timestamps on the folders to decide which order to use as the base, then append remaining bookmarks from the other side.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1435553 - Compare synced folder timestamps to determine the base child order.

Review commit: https://reviewboard.mozilla.org/r/217744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217744/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8948179 [details]
Bug 1435553 - Compare synced folder timestamps to determine the base child order.

https://reviewboard.mozilla.org/r/217744/#review223492


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/tests/sync/test_bookmark_structure_changes.js:876
(Diff revision 1)
> +    title: "J",
> +    bmkUri: "http://example.com/j",
> +  }]));
> +
> +  info("Apply remote");
> +  let observer = expectBookmarkChangeNotifications();

Error: 'observer' is assigned a value but never used. Allowed unused vars must match /^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS/. [eslint: no-unused-vars]

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8948179 [details]
Bug 1435553 - Compare synced folder timestamps to determine the base child order.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217744/diff/1-2/

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8948179 [details]
Bug 1435553 - Compare synced folder timestamps to determine the base child order.

https://reviewboard.mozilla.org/r/217744/#review223812

Seems like a bit of an edge-case, but it makes sense and isn't too much code, so yeah :)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ae31817ee1f
Compare synced folder timestamps to determine the base child order. r=markh

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2ae31817ee1f