Closed Bug 1252250 Opened 6 years ago Closed 6 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
https://hg.mozilla.org/mozilla-central/rev/bd154748b83a
Status: ASSIGNED → RESOLVED
Closed: 6 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.