1379798 - Ensure `insertTree` notifies observers with the correct parent ID

Status: RESOLVED FIXED

Description

[Lina Cambridge (she/her) [:lina]]

I noticed some "Trying to update the GUIDs cache with an invalid itemId" errors while working on bug 1366888.

Comment 1

[Lina Cambridge (she/her) [:lina]]

Attachment: Bug 1379798 - Ensure `insertTree` notifies observers with the correct parent ID.

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

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8885001 [details]
Bug 1379798 - Ensure `insertTree` notifies observers with the correct parent ID.

https://reviewboard.mozilla.org/r/155822/#review161316

Thanks for the patch. r=Standard8 with the comments addressed.

::: toolkit/components/places/Bookmarks.jsm:464
(Diff revision 1)
>          let item = insertInfos[i];
>          let itemId = itemIdMap.get(item.guid);
>          let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null;
>          // For sub-folders, we need to make sure their children have the correct parent ids.
>          let parentId;
> -        if (item.guid === parent.guid ||
> +        if (item.parentGuid === parent.guid) {

It took me a while, but I eventually worked out what was happening here - `parent` is actually the parent of the tree being inserted, not the current parent of the item.

Can you add a note here to clarify, and/or change `parent` to `treeParent` maybe?

::: toolkit/components/places/tests/bookmarks/test_bookmarks_insertTree.js:364
(Diff revision 1)
> +        title: "SeaMonkey",
> +      },
> +    ],
> +  }], guid: mozFolder.guid});
> +
> +  PlacesUtils.bookmarks.removeObserver(obs);

Just before this, can you add in an `await PlacesTestUtils.promiseAsyncUpdates();` call please?

Although we should be complete due to the await for the insertTree, I could just see some async stuff biting us with oranges later.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_insertTree.js:369
(Diff revision 1)
> +  PlacesUtils.bookmarks.removeObserver(obs);
> +
> +  let mozFolderId = await PlacesUtils.promiseItemId(mozFolder.guid);
> +  let commFolderId = await PlacesUtils.promiseItemId(bms[1].guid);
> +  deepEqual(notifications, [{
> +    itemId: await PlacesUtils.promiseItemId(bms[0].guid),

Note: you can promiseManyItemIds, but I think the individual calls here are fine as they are slightly clearer for the test.

Comment 3

[Lina Cambridge (she/her) [:lina]]

Comment on attachment 8885001 [details]
Bug 1379798 - Ensure `insertTree` notifies observers with the correct parent ID.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07faa05d6e82
Ensure `insertTree` notifies observers with the correct parent ID. r=standard8

Comment 5

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/07faa05d6e82