Ensure `insertTree` notifies observers with the correct parent ID

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

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

Comment 2

2 years ago
mozreview-review
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.
Attachment #8885001 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)

Comment 4

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07faa05d6e82
Ensure `insertTree` notifies observers with the correct parent ID. r=standard8

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07faa05d6e82
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.