Closed Bug 1474033 Opened 4 years ago Closed 4 years ago
To Folder` should bump the change counter for items moved between different parents
Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents.
46 bytes, text/x-phabricator-request
|Details | Review|
3.65 KB, patch
|Details | Diff | Splinter Review|
`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.
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.
We might want a simpler patch for Beta that just copies the change counter handling into `moveBookmark`, instead of consolidating the two.
[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 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
Attachment #8990479 - Flags: review+
Pushed by firstname.lastname@example.org: 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
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.
Attachment #8990762 - Flags: review?(standard8) → review+
Brindusa, can you verify the fix? Thanks! STR in comment 8.
I have verified the fix on the latest Nightly 63.0a1(20180718100918): the bookmarks/folders are correctly synced between devices.
Comment on attachment 8990762 [details] [diff] [review] beta.patch Fix for regression in 62, verified in nightly. Let's uplift for beta 10.
Attachment #8990762 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
You need to log in before you can comment on or make changes to this bug.