1338668 - Ignore tag query folder IDs when comparing client and server bookmark records

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler [:lina]]

Tag queries are special because we rewrite them to point to the local tag folder ID. It's expected that the folders won't match, but all other params should.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records.

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

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8836213 [details]
Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records.

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

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8836213 [details]
Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records.

https://reviewboard.mozilla.org/r/111674/#review112968

Two nits and one concern that might be misplaced. Looks fine to me.

::: services/sync/modules/bookmark_validator.js:28
(Diff revision 1)
>  function isNodeIgnored(treeNode) {
>    return treeNode.annos && treeNode.annos.some(anno => anno.name == LEFT_PANE_ROOT_ANNO ||
>                                                         anno.name == LEFT_PANE_QUERY_ANNO);
>  }
> +
> +function areArraysEqual(a, b) {

maybe move to Utils? Your call.

::: services/sync/modules/bookmark_validator.js:40
(Diff revision 1)
> +    }
> +  }
> +  return true;
> +}
> +
> +function areURLsEqual(a, b) {

Do we care about case in this function? (answer may be "yes", in which case it's fine as is)

::: services/sync/modules/bookmark_validator.js:69
(Diff revision 1)
> +  }
> +  for (let key of aKeys) {
> +    if (!bKeys.has(key)) {
> +      return false;
> +    }
> +    if (key == "folder") {

if a has folder=TOOLBAR and b has folder=BOOKMARKS_MENU we'll say they're the same. ISTM like we filter out the ones that are numeric ids.

I could be wrong though.

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8836213 [details]
Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records.

https://reviewboard.mozilla.org/r/111674/#review112968

> Do we care about case in this function? (answer may be "yes", in which case it's fine as is)

Yes, I think so. We might not care about trailing slashes, default ports, etc. but I opted to leave those in for now.

> if a has folder=TOOLBAR and b has folder=BOOKMARKS_MENU we'll say they're the same. ISTM like we filter out the ones that are numeric ids.
> 
> I could be wrong though.

That's a good point. In theory, those aren't tag folders, so something fishy is going on if they show up in a `RESULTS_AS_TAG_CONTENTS` query. In practice, I don't trust my theory, so changing to follow your suggestion.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8836213 [details]
Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cb7e2608b1d
Ignore tag query folder IDs when comparing client and server bookmark records. r=tcsc

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2cb7e2608b1d