Bug 1496878 - Ensure the bookmark merger flags newer locally moved children for reupload. r?markh,tcsc
47 bytes, text/x-phabricator-request
|Details | Review|
Leif's fantastic analysis of the validation data from the new bookmarks engine (https://gist.github.com/irrationalagent/2dd2d50eb8d738b353fb1fb95144d0f5) shows that `parentid` differences are much more frequent for the new engine. I looked into this in Dogear, and have a theory. :-) When we prefer a local move over a remote move, we give the merged parent a new structure state, but not the merged child. (We only do that for orphans). However, older Desktops and Android use the child's `parentid` to determine the parent, not the parent's `children`. Syncing with another Desktop using the new engine will still work, since it ignores `parentid`. However, syncing with an old Desktop, or with Android, will cause bad things to happen. We still accidentally did the right thing for items where we took the local value state, since we didn't reset `syncChangeCounter` in that case. However, if we had a newer remote value with a new structure, we'd reset the local change flags and not upload the child...even if it had the right flag before merging.
Whew, this turned into a yak shave! 🐮 Hopefully the patch clarifies the behavior.
This commit: * Changes the merger to flag locally moved children for reupload, along with their parents, if the local move is newer. * Replaces `valueState` and `structureState` in the merge states table with simpler flags. Rows with `useRemote` need to be updated in Places; rows with `shouldUpload` need change counter bumps. * `itemsToMerge` now accounts for local-only items with a `LEFT JOIN`. * Uses `shouldUpload` from the merged tree to set the change counters for all items, instead of relying on existing values and the `flagNewStructure` trigger to handle items with new structure. * Fixes keyword reuploading to handle the above, and match the non-mirror behavior. * Removes `structureCounts.new` from telemetry. Any merge with items added to the same folder on both sides will count as new structure, so it's not especially interesting. MozReview-Commit-ID: EktIBQN9FSY Depends on D11119
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/be95dcc90365 Ensure the bookmark merger flags newer locally moved children for reupload. r=tcsc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.