Closed Bug 1474033 Opened 4 years ago Closed 4 years ago

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


(Toolkit :: Places, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified


(Reporter: lina, Assigned: lina)



(Keywords: regression)


(2 files)

`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.
Duplicate of this bug: 1472114
[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.
Attachment #8990479 - Flags: review+
Pushed by
Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8
Attached patch beta.patchSplinter Review
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)
Attachment #8990762 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1474256
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #8990762 - Flags: review?(standard8) → review+
Brindusa, can you verify the fix? Thanks! STR in comment 8.
Flags: needinfo?(brindusa.tot)
I have verified the fix on the latest Nightly 63.0a1(20180718100918): the bookmarks/folders are correctly synced between devices.
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
Comment on attachment 8990762 [details] [diff] [review]

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.
Flags: qe-verify+
Thanks, everyone!
No longer blocks: 1472965
Duplicate of this bug: 1472965
You need to log in before you can comment on or make changes to this bug.