Closed Bug 1252250 Opened 9 years ago Closed 9 years ago

Implement browser.bookmarks.removeTree()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Implement browser.bookmarks.removeTree(), which is documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/removeTree.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 47.3 - Mar 7
Flags: blocking-webextensions+
I think there is a problem with the current API. It looks like the current implementation of `remove` [1] / [2] actually behaves like `removeTree` [3], as calling `remove` on a folder removes it and all of its contents (including bookmarks and subfolders). This is because `removeBookmark` [2] calls `removeFoldersContents` [4] if the item is a folder. According to the spec [5], which I verified in Canary, `remove` should remove an empty folder, but calling it on a non-empty folder should result in an exception. Therefore, I think this bug should really be about: 1. Implement `removeTree` in ext-bookmarks.js and simply have it use the existing logic for `remove` in Bookmarks.jsm. 2. Change `remove` in ext-bookmarks.js so that it cannot remove a non-empty folder. I think the logic for #2 should be in Bookmarks.jsm, which seems to mean changing the behaviour of `removeBookmark` [2]. I'm not sure how to best address that as I'm not sure why the current behaviour exists. Do we need new methods (and/or do existing methods need to be renamed), or should we provide an argument to `remove` and/or `removeBookmark` to specify whether folder contents should be removed? Will any changes we make cause problems for existing consumers of Bookmarks.jsm? [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#381 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1025 [3] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Bookmarks/removeTree [4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1475 [5] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/remove
Flags: needinfo?(mak77)
This is just an idea of how we might address this issue. I realize there are probably better ones. I chose to make emptyFoldersOnly optional and truthy so that the API can stay the same for existing consumers. Feedback is welcome. Review commit: https://reviewboard.mozilla.org/r/37453/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37453/
Attachment #8725384 - Flags: review?(kmaglione+bmo)
Note also that when running the tests I do see some errors on the screen, but all of the tests pass. I'm not sure why this is, but it doesn't seem like a good thing.
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak https://reviewboard.mozilla.org/r/37453/#review34023 For the most part, this looks good, but it looks like it prevents `.remove()` from removing empty folders, which it shouldn't. Also, please request review from a Places peer for the Bookmarks.jsm bits. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:158 (Diff revision 1) > - > + }).then(results => { We shouldn't need a separate handler here. And, in this case, it won't get a `results` argument. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:159 (Diff revision 1) > + Promise.resolve().then(() => { This line needs a `return`. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:321 (Diff revision 1) > + browser.bookmarks.search({title: "Mozilla Folder"}).then(result => { This also needs a `return` ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:333 (Diff revision 1) > browser.test.fail(`Error: ${String(error)} :: ${error.stack}`); There should also be a `notifyFail("bookmarks")` here, or the test will just hang until it times out. ::: toolkit/components/places/Bookmarks.jsm:375 (Diff revision 1) > + * @param {boolean} [optional] emptyFoldersOnly JSDoc specifies optional arguments by surrounding the argument name in square brackets. This name seems to suggest that it can only accept folders. ::: toolkit/components/places/Bookmarks.jsm:1038 (Diff revision 1) > + throw new Error("Cannot remove a non-empty folder."); The seems to also exclude empty folders.
Attachment #8725384 - Flags: review?(kmaglione+bmo)
please, instead of passing new arguments inline, pass an options optional object, so it's easy in future to add further options.
Flags: needinfo?(mak77)
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37453/diff/1-2/
Attachment #8725384 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/37453/#review34023 > This line needs a `return`. I added that, but then I had to move the return that was later (now line 166) inside the `then`. This seems a bit ugly to me, but maybe is what is expected? Would it be better to add another `then` just in order to outdent that later `return`? > This also needs a `return` Same comment as above. I'm not crazy about where the subsequent return ended up. > JSDoc specifies optional arguments by surrounding the argument name in square brackets. > > This name seems to suggest that it can only accept folders. I also followed mak's advice and changed this into an `options` object. > The seems to also exclude empty folders. Good catch. I meant to add the logic for `_childCount` but forgot to. I've now added a test for this which also caught the bug before I fixed it.
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak Requesting review on the Bookmarks.jsm code.
Attachment #8725384 - Flags: review?(mak77)
Attachment #8725384 - Flags: review?(mak77)
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37453/diff/2-3/
mak, I just added a test for the new functionality of remove in Bookmarks.jsm. The test seems to be doing what I want it to, but `Assert.throws` is not catching the exception, and I'm not sure why.
Attachment #8725384 - Flags: review?(mak77)
(In reply to Bob Silverberg [:bsilverberg] from comment #10) > mak, I just added a test for the new functionality of remove in > Bookmarks.jsm. The test seems to be doing what I want it to, but > `Assert.throws` is not catching the exception, and I'm not sure why. The exception is converted to a rejected promise, so you need to use Assert.rejects instead.
yep, Assert.throws doesn't yield nor wait
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak https://reviewboard.mozilla.org/r/37453/#review34173 r=me for the WebExtension parts. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:329 (Diff revision 3) > + return browser.bookmarks.create({title: "Empty Folder"}); This should probably have its own top-level `.then()` clause. ::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:335 (Diff revision 3) > + browser.bookmarks.remove(emptyFolderId).then(() => { `return`
Attachment #8725384 - Flags: review?(kmaglione+bmo) → review+
Attachment #8725384 - Attachment description: MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r?kmag → MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r?kmag r?mak
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37453/diff/3-4/
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak https://reviewboard.mozilla.org/r/37453/#review34369 Looks great! ::: toolkit/components/places/Bookmarks.jsm:1030 (Diff revision 4) > -function removeBookmark(item) { > +function removeBookmark(item, options={}) { doesn't look like we need a default value here, this is only invoked by remove() that already provides one.
Attachment #8725384 - Flags: review?(mak77) → review+
Comment on attachment 8725384 [details] MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37453/diff/4-5/
Attachment #8725384 - Attachment description: MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r?kmag r?mak → MozReview Request: Bug 1252250 - Implement browser.bookmarks.removeTree(), r=kmag r=mak
That last try run looked pretty bad, so I cancelled it and submitted a new one https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d015d4ed67e
Lots of bustage on Win 7 again, although it doesn't _seem_ to be related to the patch. Pushing to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c2825731cf
I keep seeing the same multiple problems on Win 7 on try. Kris, do you have any suggestions about this? I thought maybe it was an infrastructure intermittent, but it seems to happen on every push.
Flags: needinfo?(kmaglione+bmo)
That's been happening on most try runs lately, not just yours. I don't know what the problem is, but it's not related to your patch.
Flags: needinfo?(kmaglione+bmo)
Blocks: 1253652
No longer blocks: 1253652
Keywords: checkin-needed
Hi Bob! I checked your patch in on top of bug 1251269, but looks like the merge got a bit goofy, so I backed it out. Could you provide a rebased patch, please? Sorry for the noise.
Flags: needinfo?(bob.silverberg)
I fixed the conflicts and moved the `notifyPass` call to its own handler to simplify future merges.
Flags: needinfo?(bob.silverberg)
Thanks Kris
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: