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

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

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

Updated

3 years ago
No longer depends on: 1252250
Assignee

Comment 1

3 years ago
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/
Attachment #8726845 - Flags: review?(mak77)
Attachment #8726845 - Flags: review?(kmaglione+bmo)
Assignee

Updated

3 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Blocks: 1236121
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.
Attachment #8726845 - Flags: review?(kmaglione+bmo)
Assignee

Updated

3 years ago
Iteration: 47.3 - Mar 7 → 48.1 - Mar 21
Assignee

Comment 3

3 years ago
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/
Attachment #8726845 - Flags: review?(kmaglione+bmo)
Assignee

Comment 4

3 years ago
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 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.
Attachment #8726845 - Flags: review?(mak77)
Assignee

Updated

3 years ago
Attachment #8726845 - Flags: review?(mak77)
Assignee

Comment 6

3 years ago
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/
Assignee

Comment 7

3 years ago
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.
Assignee

Comment 8

3 years ago
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.
(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 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
Attachment #8726845 - Flags: review?(mak77) → review+
Assignee

Comment 11

3 years ago
(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
(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.
(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.
(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 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.
Attachment #8726845 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 16

3 years ago
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/
Assignee

Comment 17

3 years ago
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.
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))`
Assignee

Comment 19

3 years ago
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/
Assignee

Comment 20

3 years ago
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.
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);
Assignee

Comment 22

3 years ago
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/
Assignee

Comment 23

3 years ago
https://reviewboard.mozilla.org/r/38267/#review35647

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

Ahhhhhhhh!
Assignee

Comment 25

3 years ago
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/
Assignee

Comment 27

3 years ago
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.
Assignee

Comment 28

3 years ago
The try for this seems to be stalled on Win 8, but everything else looks fine, so I think this is good to checkin.
Keywords: checkin-needed

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/282a763e8fe7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.