1474033 - `moveToFolder` should bump the change counter for items moved between different parents

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Lina Butler (ex-Mozilla)]

`PlacesUtils.bookmarks.update` already does this, but we need to give `moveToFolder` the same treatment. Otherwise, we won't reupload the moved children, and their `parentid` won't be updated on the server.

For the mirror (and iOS), that's OK, since we treat the parent's `children` as canonical. But this will confuse the older bookmarks engine and Android, both of which treat the item's `parentid` as canonical.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents.

This patch unifies `updateBookmark` and `moveBookmark`.
`update` already handles the Sync change counter, and `move` specially
optimizes for moves. We can consolidate the two by reusing the queries
from `move` in `update`, moving its extra `setAncestorsLastModified`
call to `PlacesUtils.bookmarks.update`, and removing its transaction
and wrapper.

Comment 2

[Lina Butler (ex-Mozilla)]

We might want a simpler patch for Beta that just copies the change counter handling into `moveBookmark`, instead of consolidating the two.

Comment 4

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bebcc69bd1917a7fc9f160b028140edad86a365f

Comment 5

[Lina Butler (ex-Mozilla)]

[Tracking Requested - why for this release]: Fixes an issue where moving a bookmark between two folders doesn't sync the move to other devices.

Comment 6

[Phabricator Automation]

Comment on attachment 8990479 [details]
Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents.

Mark Banner (:standard8) has approved the revision.

https://phabricator.services.mozilla.com/D2016

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4e025f18962
Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8

Comment 8

[Lina Butler (ex-Mozilla)]

Attachment: beta.patch

Patch for Beta.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1404909.
[User impact if declined]: Broken bookmark sync: bookmarks moved into different folders on one device won't be moved on others.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, please.

1. Set up Sync on two devices (one Desktop, the other Desktop or mobile).
2. Add a bookmark on one device, verify it's synced to the second device.
3. Move the bookmark into a different folder on the first device, sync, and verify it's moved into the same folder on the second.

[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Small patch that copy-pastes a condition from an existing, tested function; also includes new xpcshell test.
[String changes made/needed]: None.

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a4e025f18962

Comment 11

[Lina Butler (ex-Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45b9fb44c97683d524a6e6abc7f1c4b7cfefa35

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Brindusa, can you verify the fix? Thanks! STR in comment 8.

Comment 13

[Gyula Palko[:gpalko]]

I have verified the fix on the latest Nightly 63.0a1(20180718100918): the bookmarks/folders are correctly synced between devices.

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8990762 [details] [diff] [review]
beta.patch

Fix for regression in 62, verified in nightly. Let's uplift for beta 10.

Comment 15

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6aedd84a5388

Comment 16

[David Olah]

I tested this on Windows 10 x64, Ubuntu 16.04 and Mac OS following the steps from comment 8 on Beta 62.0b10 and Nightly 63.0a1(2018-07-18) and the issue is no longer reproducible. 
Based on the information above, I will mark this as verified-fixed on 62.

Comment 17

[Lina Butler (ex-Mozilla)]

Thanks, everyone!