Closed Bug 1474033 Opened Last year Closed Last year

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

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: lina, Assigned: lina)

References

Details

(Keywords: regression)

Attachments

(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.

https://phabricator.services.mozilla.com/D2016
Attachment #8990479 - Flags: review+
Pushed by kcambridge@mozilla.com:
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
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
https://hg.mozilla.org/mozilla-central/rev/a4e025f18962
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attachment #8990762 - Flags: review?(standard8) → review+
Flags: qe-verify+
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]
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.
Flags: qe-verify+
Status: RESOLVED → VERIFIED
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.