1253652 - Fix browser.bookmarks.move() and add tests for it

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

The current implementation of browser.bookmarks.move() is broken. This bug is to fix it and add coverage for it.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

Requesting review from mak for Bookmarks.jsm - it seems the validation check I removed is not needed, and it is causing issues for our code.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

https://reviewboard.mozilla.org/r/38267/#review34741

::: browser/components/extensions/ext-bookmarks.js:156
(Diff revision 1)
> -        if (destination.index !== null) {
> +        info.index = destination.index || Bookmarks.DEFAULT_INDEX;

I think we actually have to check for null here. Indices are 0-based.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:335
(Diff revision 1)
> +      return browser.bookmarks.move(bookmarkId, {parentId: "toolbar_____", index: 1});

We need a test that `index: 0` is treated as index 0.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:339
(Diff revision 1)
> +    });

Hm. I just realized that we don't actually delete the bookmarks we created when the test ends.

We should definitely be doing that.

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

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

Comment 4

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review34741

> Hm. I just realized that we don't actually delete the bookmarks we created when the test ends.
> 
> We should definitely be doing that.

I tried to do this using `eraseEverything` in Bookmarks.jsm [1], but I cannot seem to import it. I left my code in this patch which attempts to do `Components.utils.import("resource://gre/modules/PlacesUtils.jsm");` but that generates an exception: "Components.utils is undefined". I tried a few different syntaxes, including what I found in other tests (e.g., [2] and [3]) but I cannot seem to get it to recognize `utils` in `Components`. What do I need to do differently?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#441
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html#23
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html#21

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

https://reviewboard.mozilla.org/r/38267/#review35055

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:321
(Diff revision 1)
> +      browser.bookmarks.getChildren("menu________"),

fwiw, even if they are not directly supported by the webExtension API, it would be nicer if you would use PlacesUtils.bookmarks.xxxGuid in test_ext_bookmarks.html instead of directly hardcoding those guids.
You should be able to use SpecialPowers.loadChromeScript (bug 1251139 will make that even simpler).

Note, this can be done in a follow-up bug, for now I'd just define some const at the top of the js.

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 1)
> -      , parentGuid: { requiredIf: b => b.hasOwnProperty("index") }

I think you may need to update the update test in places/tests/bookmarks for this change.

Fwiw, the reason to require the parent in any case, was to avoid consumers thinking this was good to implement a sorting function, since htat would be unefficient and we have a specific api for that.
It's a weak reason though, so I'm ok with removing it.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

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

Comment 7

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review35055

> fwiw, even if they are not directly supported by the webExtension API, it would be nicer if you would use PlacesUtils.bookmarks.xxxGuid in test_ext_bookmarks.html instead of directly hardcoding those guids.
> You should be able to use SpecialPowers.loadChromeScript (bug 1251139 will make that even simpler).
> 
> Note, this can be done in a follow-up bug, for now I'd just define some const at the top of the js.

I addressed this for now by adding const as you suggested.

> I think you may need to update the update test in places/tests/bookmarks for this change.
> 
> Fwiw, the reason to require the parent in any case, was to avoid consumers thinking this was good to implement a sorting function, since htat would be unefficient and we have a specific api for that.
> It's a weak reason though, so I'm ok with removing it.

I updated the tests in two ways:
1. Removed the check for invalid input that required a `parentGuid` when updating `index`.
2. Updated the test that updates the `index` so that one of the updates is done without specifying a `parentGuid` to test that it actually works.

Comment 8

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review34741

> I tried to do this using `eraseEverything` in Bookmarks.jsm [1], but I cannot seem to import it. I left my code in this patch which attempts to do `Components.utils.import("resource://gre/modules/PlacesUtils.jsm");` but that generates an exception: "Components.utils is undefined". I tried a few different syntaxes, including what I found in other tests (e.g., [2] and [3]) but I cannot seem to get it to recognize `utils` in `Components`. What do I need to do differently?
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#441
> [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html#23
> [3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html#21

Note that I have commented out the cleanup code in my latest push so the tests can run. I still need some advice on how to call `Bookmarks.eraseEverything` from the test cleanup.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Note that I have commented out the cleanup code in my latest push so the
> tests can run. I still need some advice on how to call
> `Bookmarks.eraseEverything` from the test cleanup.

SpecialPowers.loadChromeScript may help also for this case.
Out of curiosity, do you have a special setup for these tests? How do they run with chrome privilege?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

https://reviewboard.mozilla.org/r/38267/#review35343

Comment 11

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #9)
> 
> SpecialPowers.loadChromeScript may help also for this case.
> Out of curiosity, do you have a special setup for these tests? How do they
> run with chrome privilege?

I don't know, and perhaps they don't which is the problem. I am guessing that if they were chrome mochitests then it would be possible, and that's probably why [1] can do it, but I'm not sure why one chooses chrome vs. non-chrome mochitests.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html#23

Comment 12

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #11)
> I don't know, and perhaps they don't which is the problem.

And the webExtension API is exposed to content? that would be scary.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Marco Bonardo [::mak] from comment #9)
> Out of curiosity, do you have a special setup for these tests? How do they
> run with chrome privilege?

We have helpers that call out to the parent process using SpecialPowers, and have it load temporary add-ons.

(In reply to Marco Bonardo [::mak] from comment #12)
> And the webExtension API is exposed to content? that would be scary.

No, we only have the indirect ability to communicate with temporary add-ons, and only through SpecialPowers.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Note that I have commented out the cleanup code in my latest push so the
> tests can run. I still need some advice on how to call
> `Bookmarks.eraseEverything` from the test cleanup.

We should probably just clean them up using the `browser.bookmarks` API and then check that everything's sane afterwards.

If you need to interact with Bookmarks.jsm directly, it has to be done from the parent process, which means we'd have to make this a chrome mochitest (which shouldn't be a problem).

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

https://reviewboard.mozilla.org/r/38267/#review35415

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:18
(Diff revision 3)
> +//SimpleTest.registerCleanupFunction(() => { PlacesUtils.bookmarks.eraseEverything(); });

We can deal with this in a follow-up bug. For now, please just make sure that the bookmark that you move out of "Mozilla Folder" in this test gets deleted when you're done with it.

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38267/diff/3-4/

Comment 17

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review35415

> We can deal with this in a follow-up bug. For now, please just make sure that the bookmark that you move out of "Mozilla Folder" in this test gets deleted when you're done with it.

Okay, I added a bunch of code to track bookmarks that have been created and not removed, and then remove them at the end of the test. It's pretty ugly, but it seems to work. Of course it won't work if there is an exception during the test, as it's not an official cleanupFunction, but I guess it will do for now. I know you r+'d this, but I'd like you to take a look at this new code I added because I think it's kind of questionable.

Comment 18

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/38267/#review35647

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:19
(Diff revisions 3 - 4)
> +  let createdBookmarks = new Map();

This can be a `Set`.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:405
(Diff revisions 3 - 4)
> +        createdBookmarks.delete(bookmark.id);

Rather than delete these, I'd just skip adding them in the first place. You can add the bookmark you move out of "Mozilla Folder" just before you move it.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:440
(Diff revisions 3 - 4)
> +      removalPromises.push(browser.bookmarks.remove(bookmarkId));

I'd go with `Array.from(createdBookmarks, guid => browser.bookmarks.remove(guid))`

Comment 19

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38267/diff/4-5/

Comment 20

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review35647

> I'd go with `Array.from(createdBookmarks, guid => browser.bookmarks.remove(guid))`

I tried to do something like that originally, but it generated exceptions, I believe because it wasn't waiting for the promises to resolve before continuing with the rest of the test (which included unloading the extension). That is why I went with the `Promise.all` approach. Maybe I'm not understanding how you are suggesting implementing this though. If I just replace all of my cleanup code with that line I get errors, like I did before.

Comment 21

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/38267/#review35647

> I tried to do something like that originally, but it generated exceptions, I believe because it wasn't waiting for the promises to resolve before continuing with the rest of the test (which included unloading the extension). That is why I went with the `Promise.all` approach. Maybe I'm not understanding how you are suggesting implementing this though. If I just replace all of my cleanup code with that line I get errors, like I did before.

let promises = Array.from(createdBookmarks,
                            guid => browser.bookmarks.remove(guid))
  return Promise.all(promises);

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38267/diff/5-6/

Comment 23

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/38267/#review35647

> let promises = Array.from(createdBookmarks,
>                             guid => browser.bookmarks.remove(guid))
>   return Promise.all(promises);

Ahhhhhhhh!

Comment 24

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2b33007b5aa

Comment 25

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8726845 [details]
MozReview Request: Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38267/diff/6-7/

Comment 26

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=375f70af08d1

Comment 27

[Bob Silverberg [:bsilverberg]]

Note that my last push to review was an attempt to proactively address merge conflicts that were likely to arise as a result of Bug 1236121 landing. I'm not sure if I've done this correctly, but I guess we'll see what try thinks of it.

Comment 28

[Bob Silverberg [:bsilverberg]]

The try for this seems to be stalled on Win 8, but everything else looks fine, so I think this is good to checkin.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/282a763e8fe7

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/282a763e8fe7