1496878 - Ensure the bookmark merger flags newer locally moved children for reupload

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler (ex-Mozilla)]

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.

Comment 1

[Lina Butler (ex-Mozilla)]

Whew, this turned into a yak shave! 🐮 Hopefully the patch clarifies the behavior.

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1496878 - Ensure the bookmark merger flags newer locally moved children for reupload. r?markh,tcsc

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be95dcc90365
Ensure the bookmark merger flags newer locally moved children for reupload. r=tcsc

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

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