1436837 - Always prefer the local value state for synced roots

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Lina Butler (ex-Mozilla)]

Currently, the mirror treat roots like any other folder, and applies changes like titles, descriptions, and creation dates. Per our conversation on IRC this morning, these kinds of changes don't make sense for roots, and we should treat them as immutable.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1436837 - Always prefer the local value state for synced roots.

Currently, the mirror treat roots like any other folder, and applies
remote value changes like titles, descriptions, and creation dates.
We should instead treat roots as immutable, and only apply new
structure states, to match the behavior of the legacy engine, iOS,
and Android.

This has a semantic change: if a root is changed locally, but we dedupe
all its children to match the remote tree, we'll still reupload a
record for the root. However, this change is consistent with the legacy
engine, as well as how we handle orphans moved into a locally modified
"unfiled".

Review commit: https://reviewboard.mozilla.org/r/218882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218882/

Comment 2

[Lina Butler (ex-Mozilla)]

Comment on attachment 8949534 [details]
Bug 1436837 - Always prefer the local value state for synced roots.

https://reviewboard.mozilla.org/r/218882/#review224620

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1906
(Diff revision 1)
>    // A view of the value states for all bookmarks in the mirror. We use triggers
>    // on this view to update Places. Note that we can't just `REPLACE INTO
>    // moz_bookmarks`, because `REPLACE` doesn't fire the `AFTER DELETE` triggers
>    // that Places uses to maintain schema coherency.
>    await db.execute(`
> -    CREATE TEMP VIEW newRemoteItems(localId, remoteId, localGuid, mergedGuid,
> +    CREATE TEMP VIEW mergedItems(localId, remoteId, hasRemoteValue, oldGuid,

While I was here, I renamed the views and column names to better reflect what they do. "New" has a specific meaning in this file, which we're not using here.

::: toolkit/components/places/tests/sync/test_bookmark_deduping.js:413
(Diff revision 1)
>    let changesToUpload = await buf.apply();
>    deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
>  
>    let idsToUpload = inspectChangeRecords(changesToUpload);
>    deepEqual(idsToUpload, {
> -    updated: [],
> +    updated: ["mobile"],

This is the semantic change I mentioned in the commit message. Since we added `bookmarkAAA1`, we bumped mobile's change counter. Even though we deduped `bookmarkAAA1` to `bookmarkAAAA`, where Places now matches the server, we still keep mobile flagged for reupload.

I think that's fine, and consistent with what we do today.

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8949534 [details]
Bug 1436837 - Always prefer the local value state for synced roots.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218882/diff/1-2/

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8949534 [details]
Bug 1436837 - Always prefer the local value state for synced roots.

https://reviewboard.mozilla.org/r/218882/#review224770

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8949534 [details]
Bug 1436837 - Always prefer the local value state for synced roots.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218882/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7c0d34808c7
Always prefer the local value state for synced roots. r=mak

Comment 7

[Cristina Coroiu [:ccoroiu]]

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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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