Closed
Bug 1379798
Opened 6 years ago
Closed 6 years ago
Ensure `insertTree` notifies observers with the correct parent ID
Categories
(Toolkit :: Places, enhancement)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
Details
Attachments
(1 file)
I noticed some "Trying to update the GUIDs cache with an invalid itemId" errors while working on bug 1366888.
Comment hidden (mozreview-request) |
Comment 2•6 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) |
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07faa05d6e82
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•