1234020 - Add Promise support to asynchronous Webextension APIs

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Johann Hofmann [:johannh]]

Many of the Webextension APIs use Promises internally and then call a callback as the end result. We should expose those Promises as well.

Supporting native Promises offers many benefits for developers and is a must for good JS API design in (almost) 2016, IMO. It's perfectly possible to support both Promises and callbacks alongside each other.

So developers can currently do this:

   chrome.bookmarks.search("mozilla", function(result){console.log(result)})

But I'd also like to enable:

   let promise = chrome.bookmarks.search("mozilla");

   promise.then(function(result){console.log(result)});

I would say that as long as we don't break the Chrome API we should be encouraged to improve upon it and provide developers even more convenience and future-proofness.

This would not break compatibility with the Chrome API, if done correctly.

Comment 1

[Kris Maglione [:kmag]]

I've been thinking about doing this for a while.

I don't think we should do it for methods accessed via the `chrome` namespace, only those accessed via the `browser` namespace. It would be nice to provide a shim script that people can use to get the same behavior in other browsers, too.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I agree. This seems like a nice thing to do that would mostly be backwards compatible.

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

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

Comment 4

[Kris Maglione [:kmag]]

I have patches on top of this that clean up the wrapper function once all of the async methods are updated to handle promises, so it winds up as:

          callAsyncFunction(ns, name, args, callback) {
            // We pass an empty stub function as a default callback for
            // the `chrome` API, so promise objects are not returned,
            // and lastError values are reported immediately.
            if (callback === null) {
              callback = defaultCallback;
            }

            let promise;
            try {
              promise = schemaApi[ns][name](...args);
            } catch (e) {
              promise = Promise.reject(e);
            }

            return context.wrapPromise(promise || Promise.resolve(), callback);
          },

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8713917 [details]
MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

https://reviewboard.mozilla.org/r/32855/#review29893

So, IIUC, an API function has to opt into this by returning a promise and ignoring any callbacks passed in. I could imagine that we could synthesize a callback for all callback-taking API functions if one isn't provided. Is there any reason why you did it this way?

::: toolkit/components/extensions/Extension.jsm:360
(Diff revision 1)
> -          return schemaApi[ns][name].apply(null, args);
> +            return schemaApi[ns][name](...args);

Do you know what Chrome sets |this| to? I'm not even sure what it will be set to here.

::: toolkit/components/extensions/Extension.jsm:391
(Diff revision 1)
> +

Extra line.

::: toolkit/components/extensions/Schemas.jsm:601
(Diff revision 1)
> +                        params.length && params[params.length - 1].name == "callback");

Heh heh. Can you file a bug to make this more explicit in the schema? Maybe a "callback" type that's basically the same as "function" or else a separate property.

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/32855/#review29893

Basically, it simplifies things if we just use promises internally everywhere. We don't have to worry about sometimes converting a callback to a promise, and sometimes converting a promise to a callback, to handle the promise variants of the API. We get lastError handling everywhere automatically. Callbacks are automatically called asynchronously without any special steps.

My plan was to update all of the API functions (aside from the ones with ambiguous args, or no schema bindings) to return promises, and then stop passing the callback.

> Do you know what Chrome sets |this| to? I'm not even sure what it will be set to here.

Well, this only applies to our API bindings, so there isn't really a correspondence to what Chrome does. In this case, it should wind up as the generated namespace object (i.e., the browser.tabs object for browser.tabs.update). Maybe we should leave it as `null` to prevent us from using `this` in those cases, but I'm not sure using `this` would be a bad thing.

> Heh heh. Can you file a bug to make this more explicit in the schema? Maybe a "callback" type that's basically the same as "function" or else a separate property.

Heh. Yeah, I'll admit it's a bit of a hack.

The schema spec forbids using any types other than the ones it defines. Maybe an `"async": "callback"` property on the method, specifying that it's an async function, and the callback arg describes the callback?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #6)
> My plan was to update all of the API functions (aside from the ones with
> ambiguous args, or no schema bindings) to return promises, and then stop
> passing the callback.

OK, seems fine.

> 
> > Do you know what Chrome sets |this| to? I'm not even sure what it will be set to here.
> 
> Well, this only applies to our API bindings, so there isn't really a
> correspondence to what Chrome does.

Oh right, I was confused.

> > Heh heh. Can you file a bug to make this more explicit in the schema? Maybe a "callback" type that's basically the same as "function" or else a separate property.
> 
> Heh. Yeah, I'll admit it's a bit of a hack.
> 
> The schema spec forbids using any types other than the ones it defines.
> Maybe an `"async": "callback"` property on the method, specifying that it's
> an async function, and the callback arg describes the callback?

Yeah, that seems fine.

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/c7dd217e7d3a42ad4ecbb4b53159fcd478dda990
Bug 1234020: Part 1 - [webext] Add initial binding-level promise<->callback support. r=billm

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8713917 [details]
MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

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

Comment 10

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie

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

Comment 11

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2c - [webext] Return promises from the idle API. r?rpl

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

Comment 12

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2d - [webext] Return promises from the runtime API. r=rpl

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

Comment 13

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2e - [webext] Return promises from the storage API. r=rpl

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

Comment 14

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie

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

Comment 15

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2g - [webext] Return promises from the browserAction API. r=rpl

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

Comment 16

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2h - [webext] Return promises from the pageAction API. r=rpl

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

Comment 17

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r?billm

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

Comment 18

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2i - [webext] Return promises from the contextMenus API. r=rpl

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

Comment 19

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2j - [webext] Return promises from the tabs API. r=rpl

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

Comment 20

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2k - [webext] Return promises from the windows API. r=rpl

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

Comment 21

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2l - [webext] Return promises from the alarms API. r=rpl

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

Comment 22

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 3 - [webext] Remove promise<->callback compatibility workarounds. r?billm

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

Comment 23

[Kris Maglione [:kmag]]

Luca, Tom, let me know if you're not OK reviewing any of these and I'll find someone else.

Luca, I know I gave you a lot, but they're mostly pretty trivial. It's mostly just a matter of making sure I didn't miss anything important, or make any obvious mistakes.

Thanks.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7dd217e7d3a

Comment 25

[Tom S [:evilpie]]

Comment on attachment 8714638 [details]
MozReview Request: Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie

https://reviewboard.mozilla.org/r/33181/#review29971

::: toolkit/components/extensions/ext-cookies.js:285
(Diff revision 1)
> -          // TODO: Set |lastError| when false.
> +          return Promise.reject({message: `Permission denied to set cookie ${JSON.stringify(details)}`});

Thanks for fixing this!

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:69
(Diff revision 1)
>    });

I am a bit concerned that we are left with no test for the callback syntax.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html:84
(Diff revision 1)
> +      if (!(options.shouldPass || options.shouldWrite)) {

I don't understand the significance of this change.

Comment 26

[Tom S [:evilpie]]

Comment on attachment 8714642 [details]
MozReview Request: Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie

https://reviewboard.mozilla.org/r/33189/#review29975

Not sure if we care about tests for the callback. This is a nice improvement.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:23
(Diff revision 1)
> -    browser.test.assertEq(bookmark.url, "http://example.org/");
> +    browser.test.assertEq("http://example.org/", bookmark.url);

Wow, why is that? I think most our test suites have actual - expected.

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8713917 [details]
MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

https://reviewboard.mozilla.org/r/32855/#review30007

This change is fine by itself, but it should not land without the corresponding change to the schema files (which is currently part of the patch "Bug 1234020: Part 2d - \[webext\] Return promises from the runtime API."), otherwise this API method will not be recognized correctly from the schema and it will not work as intended.

::: toolkit/components/extensions/ext-backgroundPage.js:136
(Diff revision 2)
> -      getBackgroundPage: function(callback) {
> +      getBackgroundPage() {

As this method is not currently covered by any of the existing test cases, none of them fails, but if this patch lands without the corresponding changes to the schema then it behaves as follows:

- if specified, the callback parameter is never called
- if omitted, an error is logged because the callback parameter is missing
- in both the cases, no promise will be returned

This API method (once it is recognized and works as expected) currently logs the following error when called:

runSafe failure: cloning into [object Window]: Error: Encountered unsupported value type writing stack-scoped structured clone
runSafeSync@resource://gre/modules/ExtensionUtils.jsm:53:86
wrapPromise/</<@resource://gre/modules/ExtensionUtils.jsm:238:22
promise callback*wrapPromise/<@resource://gre/modules/ExtensionUtils.jsm:237:9
wrapPromise@resource://gre/modules/ExtensionUtils.jsm:236:14
callAsyncFunction@resource://gre/modules/Extension.jsm:385:20
inject/stub@resource://gre/modules/Schemas.jsm:802:16
@moz-extension://c29a6ff3-c4c7-4c7a-b0ca-b6bb1f2d33ea/%7B39c11c1c-2952-4192-8d18-c1dcc60d97e1%7D.js:4:7
@moz-extension://c29a6ff3-c4c7-4c7a-b0ca-b6bb1f2d33ea/%7B39c11c1c-2952-4192-8d18-c1dcc60d97e1%7D.js:1:2

I'm not marking this error as an issue here because:

 - besides the error logged it works correctly, thanks to |runSafeSync| falling back to |runSafeSyncWithoutClone| when it fails
 - the issues was probably already there (because previously |runtime.getBackgroundPage| was explicitly calling |runSafe| instead of |runSafeWithoutClone|, and
   this API method is not currently included in any of the existent test cases)
 - the issues now happens and needs to be solved in the abstractions that are responsible of the "Promise / async callbacks" feature

so it seems like something that can be solved in a followup.

Looking into the current schema files, it seems that this one is the only API method callback where the |run*WithoutClone| variant should be used directly (because the callback parameter is intended to be non-clonable and it should not log any error)

In the schema file this callback parameter is marked as:

    ...
    "isInstanceOf": "Window",
    ...

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714639 [details]
MozReview Request: Bug 1234020: Part 2c - [webext] Return promises from the idle API. r?rpl

https://reviewboard.mozilla.org/r/33183/#review30009

::: toolkit/components/extensions/ext-idle.js:6
(Diff revision 1)
> -      queryState: function(detectionIntervalInSeconds, callback) {
> +      queryState: function(detectionIntervalInSeconds) {

This patch needs a corresponding change to the schema file "idle.json", it seems that the change to the schema is currently missing from this patch queue.

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714640 [details]
MozReview Request: Bug 1234020: Part 2d - [webext] Return promises from the runtime API. r=rpl

https://reviewboard.mozilla.org/r/33185/#review30041

How about squashing "Part 2a - [webext] Return promises from the background page APIs" into this one?
It should solve its only remaining issue (https://reviewboard.mozilla.org/r/32855/#comment39509, which exists only in case of separate landing of the two patches, and it leaves open only the followup related to the error currently logged by getBackgroundPage).

Comment 30

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33181/#review29971

> I am a bit concerned that we are left with no test for the callback syntax.

Yeah, I'm a bit on the fence about that. We do have general tests to check that the callback and promise variants behave the same, elsewhere. Ideally, it would be nice to check both for every API, in case there are corner cases, but I don't think that's going to be practical.

> I don't understand the significance of this change.

These tests make some calls which fail permissions checks. Previously they just triggered the callback without a result. Now they set lastError or reject their promise, depending on the version. This just checks that we get the correct number of failures, if we're expecting any.

Comment 31

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33189/#review29975

> Wow, why is that? I think most our test suites have actual - expected.

`assertEq` shows the first argument as the expected value. The mochitest functions do the reverse. I'm not sure why that is, but it was confusing when I was getting failures and it was showing the actual value as the expected value.

Comment 32

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33183/#review30009

> This patch needs a corresponding change to the schema file "idle.json", it seems that the change to the schema is currently missing from this patch queue.

Good catch. I did think about adding a test for this method, but it seemed silly to add a test for a stub. I guess I may as well add one now, though.

Comment 33

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/32855/#review30007

> As this method is not currently covered by any of the existing test cases, none of them fails, but if this patch lands without the corresponding changes to the schema then it behaves as follows:
> 
> - if specified, the callback parameter is never called
> - if omitted, an error is logged because the callback parameter is missing
> - in both the cases, no promise will be returned

I think I botched a rebase trying to add the flag for this API and the rest of the `extension` API in separate commits. Thanks for catching this. I'll add a test, too.

Hm. Yeah, I was planning to fix that, but I forgot.

I think the easiest thing to do is return a promise belonging to the content window to signal that a clone isn't required.

Comment 34

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33185/#review30041

The "async" flag is missing from `extension.getBackgroundPage` too. I'd rather keep the changes as separate as possible in case we have to bisect a failure at some point. Also... I don't want to have to rename all of the patches that come after it...

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714641 [details]
MozReview Request: Bug 1234020: Part 2e - [webext] Return promises from the storage API. r=rpl

https://reviewboard.mozilla.org/r/33187/#review30011

lgtm

It's a bit unfortunate that the storage API doesn't currently use a schema file, the APIs registered with a schema have:
- nicer code (e.g. because the API method can return a promise without wrapping it explicitly) 
- the APIs will automatically support the promisified version of the API call only on the "browser" API object 
  (so that any non-optional callback parameters continue to be required on the the "chrome" API object version of the async methods as expected).
- unsupported features (methods or properties) are explicitly marked

In any case this storage APIs doesn't currently validate the parameters using the schema, and unfortunately the [storage.json schema file](https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/api/storage.json) contains some fields that we do not currently support / ignore (e.g. js_module, functions), so it is better to defer discussions about these further enhancements on the storage APIs to a followup.

Comment 36

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714643 [details]
MozReview Request: Bug 1234020: Part 2g - [webext] Return promises from the browserAction API. r=rpl

https://reviewboard.mozilla.org/r/33191/#review30013

lgtm

::: browser/components/extensions/ext-browserAction.js:273
(Diff revision 1)
> +        return Promise.resolve();

It seems to me that this line fixes a bug: |setIcon| callback parameter was never called, we didn't found it before because |setIcon| is not currently covered by the existent test cases.

In a follow up issue, we should add a test case to cover setIcon callback parameter and look into possible "error scenarios" (e.g. when the callback should/shouldn't be called, what happens if the icon cannot be loaded), it can be a "good first bug" too.

Comment 37

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714644 [details]
MozReview Request: Bug 1234020: Part 2h - [webext] Return promises from the pageAction API. r=rpl

https://reviewboard.mozilla.org/r/33193/#review30089

lgtm

::: browser/components/extensions/ext-pageAction.js:235
(Diff revision 1)
> +        return Promise.resolve();

Like in browserAction, we didn't found the issue before ("the callback parameter was never called") because setIcon is not covered by any test case.

In a follow up issue, we should add a test case to cover setIcon callback parameter and look into possible "error scenarios" (e.g. when the callback should/shouldn't be called, what happens if the icon cannot be loaded), it can be a "good first bug" too.

Comment 38

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714646 [details]
MozReview Request: Bug 1234020: Part 2i - [webext] Return promises from the contextMenus API. r=rpl

https://reviewboard.mozilla.org/r/33197/#review30091

::: browser/components/extensions/ext-contextMenus.js:421
(Diff revision 1)
>        create: function(createProperties, callback) {

|contextMenus.create| doesn't seem to be covered by the current version of this patch and it support a callback parameter, as the other methods already marked and converted to async functions by this patch.

is there any issue in converting it to the new convention?

Looking in the coverate reports, it seems that the callbacks parameters are not currently covered by the existent test cases, if we decide to do not cover them in this patch (as this is already a pretty long queue of patches) we should file a followup for it.

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714647 [details]
MozReview Request: Bug 1234020: Part 2j - [webext] Return promises from the tabs API. r=rpl

https://reviewboard.mozilla.org/r/33199/#review30099

lgtm.

We have to be careful about the landing order of this patch in relation to the patches attached to Bug 1242588 and Bug 1227462 (e.g. it seems that Bug 1242588 is ready to land, and if it lands before than this patch, then the new behaviors introduced by that patch needs to be included in this one).

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714648 [details]
MozReview Request: Bug 1234020: Part 2k - [webext] Return promises from the windows API. r=rpl

https://reviewboard.mozilla.org/r/33201/#review30087

lgtm.

Unfortunately the test coverage of this module is pretty low, currently only "getAll", "getCurrent" and "update" are covered by the existent test cases:
we should file a followup which should cover the remaining methods with more test cases.

Comment 41

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714649 [details]
MozReview Request: Bug 1234020: Part 2l - [webext] Return promises from the alarms API. r=rpl

https://reviewboard.mozilla.org/r/33203/#review30105

lgtm.

Looking in the coverage reports, it seems that "get", "getAll" and "clearAll" are not currently covered by any test case, we should file a follow up for it.

As for the "storage" API, the Promises are wrapped explicitly and the compatibility behaviour on the "chrome" API methods is not recognized, because "alarms" is one of those APIs which isn't currently registered with a corresponding schema file.
Even more annoying is that in Chrome the "alarms" API is not described by a json file but using the following idl file https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/api/alarms.idl.

we should discuss in a followup how we want to handle this corner case.

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

Another issue that I found (but it is not related to one of the attached patches in particular) is that the notification API is currently missing from the set of APIs updated  to the new conventions by the current queue of patches.

is there any issue in converting the notification API methods (e.g. notification.create/clear/getAll) to the new conventions?

Comment 43

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks Kris! this is a very nice enhancement (and a great effort) both internally (the API methods code is definitely nicer) and externally (being able to get a Promise from the API methods without the wrapping them manually it is a very nice addition).

My apologies for the long time I needed to complete the review (this was my first mozreview experience as a reviewer)

Comment 44

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33197/#review30091

> |contextMenus.create| doesn't seem to be covered by the current version of this patch and it support a callback parameter, as the other methods already marked and converted to async functions by this patch.
> 
> is there any issue in converting it to the new convention?

`contextMenus.create` returns a value, so it can't also return a promise. It's unfortunate, but there's not really anything we can do about it.

Comment 45

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33193/#review30089

> Like in browserAction, we didn't found the issue before ("the callback parameter was never called") because setIcon is not covered by any test case.
> 
> In a follow up issue, we should add a test case to cover setIcon callback parameter and look into possible "error scenarios" (e.g. when the callback should/shouldn't be called, what happens if the icon cannot be loaded), it can be a "good first bug" too.

This is covered by part 2h.1

Comment 46

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33191/#review30013

> It seems to me that this line fixes a bug: |setIcon| callback parameter was never called, we didn't found it before because |setIcon| is not currently covered by the existent test cases.
> 
> In a follow up issue, we should add a test case to cover setIcon callback parameter and look into possible "error scenarios" (e.g. when the callback should/shouldn't be called, what happens if the icon cannot be loaded), it can be a "good first bug" too.

There are tests for `setIcon`, but none of them check the callback.

Comment 47

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33201/#review30087

This is bug 1190331.

Comment 48

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33203/#review30105

Alarms test coverage is bug 1190320.

I think Bill found a schema for the storage API in an older version of Chromium and was working on importing it.

Comment 49

[Kris Maglione [:kmag]]

(In reply to Luca Greco [:rpl] from comment #42)
> Another issue that I found (but it is not related to one of the attached
> patches in particular) is that the notification API is currently missing
> from the set of APIs updated  to the new conventions by the current queue of
> patches.
> 
> is there any issue in converting the notification API methods (e.g.
> notification.create/clear/getAll) to the new conventions?

Hm. I must have lost a patch. I definitely remember implementing that.

Comment 50

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714646 [details]
MozReview Request: Bug 1234020: Part 2i - [webext] Return promises from the contextMenus API. r=rpl

https://reviewboard.mozilla.org/r/33197/#review30155

Comment 51

[Kris Maglione [:kmag]]

(mid-aired myself and wound up not clearing the needinfo)

Comment 52

[Kris Maglione [:kmag]]

I don't think there's any way around a flood of mozreview spam at this point...

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8713917 [details]
MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

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

Comment 54

[Kris Maglione [:kmag]]

Comment on attachment 8714638 [details]
MozReview Request: Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie

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

Comment 55

[Kris Maglione [:kmag]]

Comment on attachment 8714639 [details]
MozReview Request: Bug 1234020: Part 2c - [webext] Return promises from the idle API. r?rpl

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

Comment 56

[Kris Maglione [:kmag]]

Comment on attachment 8714640 [details]
MozReview Request: Bug 1234020: Part 2d - [webext] Return promises from the runtime API. r=rpl

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

Comment 57

[Kris Maglione [:kmag]]

Comment on attachment 8714641 [details]
MozReview Request: Bug 1234020: Part 2e - [webext] Return promises from the storage API. r=rpl

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

Comment 58

[Kris Maglione [:kmag]]

Comment on attachment 8714642 [details]
MozReview Request: Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie

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

Comment 59

[Kris Maglione [:kmag]]

Comment on attachment 8714643 [details]
MozReview Request: Bug 1234020: Part 2g - [webext] Return promises from the browserAction API. r=rpl

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

Comment 60

[Kris Maglione [:kmag]]

Comment on attachment 8714644 [details]
MozReview Request: Bug 1234020: Part 2h - [webext] Return promises from the pageAction API. r=rpl

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

Comment 61

[Kris Maglione [:kmag]]

Comment on attachment 8714645 [details]
MozReview Request: Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r?billm

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

Comment 62

[Kris Maglione [:kmag]]

Comment on attachment 8714646 [details]
MozReview Request: Bug 1234020: Part 2i - [webext] Return promises from the contextMenus API. r=rpl

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

Comment 63

[Kris Maglione [:kmag]]

Comment on attachment 8714647 [details]
MozReview Request: Bug 1234020: Part 2j - [webext] Return promises from the tabs API. r=rpl

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

Comment 64

[Kris Maglione [:kmag]]

Comment on attachment 8714648 [details]
MozReview Request: Bug 1234020: Part 2k - [webext] Return promises from the windows API. r=rpl

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

Comment 65

[Kris Maglione [:kmag]]

Comment on attachment 8714649 [details]
MozReview Request: Bug 1234020: Part 2l - [webext] Return promises from the alarms API. r=rpl

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

Comment 66

[Kris Maglione [:kmag]]

Comment on attachment 8714650 [details]
MozReview Request: Bug 1234020: Part 3 - [webext] Remove promise<->callback compatibility workarounds. r?billm

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

Comment 67

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1234020: Part 2m - [webext] Return promises from the notifications API. r?rpl

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

Comment 68

[Tom S [:evilpie]]

Comment on attachment 8714638 [details]
MozReview Request: Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie

https://reviewboard.mozilla.org/r/33181/#review30457

Comment 69

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/56a0bd3479b00a95c485a382f0c711d9c0a45308
Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r=evilpie

https://hg.mozilla.org/integration/fx-team/rev/d10ae45a04b90d8d2e82f791471b976969e26fb8
Bug 1234020: Part 2d - [webext] Return promises from the runtime API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/bcdd50f6c6e29e26d5e43e415992d2c416187642
Bug 1234020: Part 2e - [webext] Return promises from the storage API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/efc9c545955f811d5c67374b107fc356ece76f2b
Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie

https://hg.mozilla.org/integration/fx-team/rev/3fe69369f5cadddc86914c95f1daaf8575ec2451
Bug 1234020: Part 2g - [webext] Return promises from the browserAction API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/f4165f2acab3787b0ae794596cdbee37a364ded8
Bug 1234020: Part 2h - [webext] Return promises from the pageAction API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/20be24e23d32840210a6a7f148cecfc72f5e59f4
Bug 1234020: Part 2i - [webext] Return promises from the contextMenus API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/b08e1e68221ecdbb514e3e2108f089e369cf8069
Bug 1234020: Part 2j - [webext] Return promises from the tabs API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/d30cd52d8dbef9bccd19ac276e0e7156655a4ce7
Bug 1234020: Part 2k - [webext] Return promises from the windows API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/8abc7cc3c4e68d163d1aeb1f78c57eb402adacdb
Bug 1234020: Part 2l - [webext] Return promises from the alarms API. r=rpl

Comment 70

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8713917 [details]
MozReview Request: Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r?rpl

https://reviewboard.mozilla.org/r/32855/#review30527

Comment 71

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8714639 [details]
MozReview Request: Bug 1234020: Part 2c - [webext] Return promises from the idle API. r?rpl

https://reviewboard.mozilla.org/r/33183/#review30529

Comment 72

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8716017 [details]
MozReview Request: Bug 1234020: Part 2m - [webext] Return promises from the notifications API. r?rpl

https://reviewboard.mozilla.org/r/33677/#review30531

Comment 73

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/40ddb075bbc68428d90113ccc3c2ff8d4483b60d
Bug 1234020: Part 2a - [webext] Return promises from the background page APIs. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/28e7dbc3fe9bb0ed28465b5b4e858efcc027b3af
Bug 1234020: Part 2c - [webext] Return promises from the idle API. r=rpl

https://hg.mozilla.org/integration/fx-team/rev/4666d726856dee140633ca99cf1d13809b689430
Bug 1234020: Part 2m - [webext] Return promises from the notifications API. r=rpl

Comment 74

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56a0bd3479b0
https://hg.mozilla.org/mozilla-central/rev/d10ae45a04b9
https://hg.mozilla.org/mozilla-central/rev/bcdd50f6c6e2
https://hg.mozilla.org/mozilla-central/rev/efc9c545955f
https://hg.mozilla.org/mozilla-central/rev/3fe69369f5ca
https://hg.mozilla.org/mozilla-central/rev/f4165f2acab3
https://hg.mozilla.org/mozilla-central/rev/20be24e23d32
https://hg.mozilla.org/mozilla-central/rev/b08e1e68221e
https://hg.mozilla.org/mozilla-central/rev/d30cd52d8dbe
https://hg.mozilla.org/mozilla-central/rev/8abc7cc3c4e6
https://hg.mozilla.org/mozilla-central/rev/40ddb075bbc6
https://hg.mozilla.org/mozilla-central/rev/28e7dbc3fe9b
https://hg.mozilla.org/mozilla-central/rev/4666d726856d

Comment 75

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8714645 [details]
MozReview Request: Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r?billm

https://reviewboard.mozilla.org/r/33195/#review30807

::: browser/components/extensions/ext-bookmarks.js:169
(Diff revision 2)
> -          return Bookmarks.update(info).then(convert);
> +        return Bookmarks.update(info).then(convert);

It looks like there's a case where update will throw directly. Can that happen in our case?

::: browser/components/extensions/ext-bookmarks.js:178
(Diff revision 2)
> -          return Bookmarks.remove(info).then(result => {});
> +        return Bookmarks.remove(info).then(result => {});

Same here.

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js:249
(Diff revision 2)
> +                    browser.test.succeed("setIcon with invalid icon size");
> +                  }));
>              } catch (e) {
>                browser.test.succeed("setIcon with invalid icon size");

This seems a little odd. We'll accept either an immediate exception or a promise rejection. I'd rather we define one or the other as the correct behavior here.

Comment 76

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> It looks like there's a case where update will throw directly. Can that happen in our case?

Oh, I guess we have a try/catch block in Extension.jsm that will handle this case. Is that right? It is a little worrisome to me that exceptions in our own code are passed onto the extension. I'd rather be able to say that such exceptions are always bugs in our code, and require ext-*.js code to always reject if it wants to expose a failure to the extension. But I guess I don't feel that strongly.

Comment 77

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8714650 [details]
MozReview Request: Bug 1234020: Part 3 - [webext] Remove promise<->callback compatibility workarounds. r?billm

https://reviewboard.mozilla.org/r/33205/#review30813

Comment 78

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #76)
> > It looks like there's a case where update will throw directly. Can that happen in our case?
> 
> Oh, I guess we have a try/catch block in Extension.jsm that will handle this
> case. Is that right? It is a little worrisome to me that exceptions in our
> own code are passed onto the extension. I'd rather be able to say that such
> exceptions are always bugs in our code, and require ext-*.js code to always
> reject if it wants to expose a failure to the extension. But I guess I don't
> feel that strongly.

Yeah, I'm kind of on the fence about that. I'm leaning towards passing through any exceptions that belong to the extension scope and translating anything else into "An unexpected error has occurred", mainly because I don't think the error messages will make sense when the stacks wind up pointing at the closest bit of extension code rather than where the error actually happened.

Comment 79

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #75)
> ::: browser/components/extensions/ext-bookmarks.js:169
> (Diff revision 2)
> > -          return Bookmarks.update(info).then(convert);
> > +        return Bookmarks.update(info).then(convert);
> 
> It looks like there's a case where update will throw directly. Can that
> happen in our case?

Yeah, at least in the case of an invalid GUID. I'm not sure whether passing the error message is the right thing to do here. It has more about what went wrong than any error message we can come up with, but it also uses different property names than we use.

> :::
> browser/components/extensions/test/browser/
> browser_ext_browserAction_pageAction_icon.js:249
> (Diff revision 2)
> > +                    browser.test.succeed("setIcon with invalid icon size");
> > +                  }));
> >              } catch (e) {
> >                browser.test.succeed("setIcon with invalid icon size");
> 
> This seems a little odd. We'll accept either an immediate exception or a
> promise rejection. I'd rather we define one or the other as the correct
> behavior here.

I think this fixed now that the manifest schema changes have landed. It should always just throw an error now.

Comment 80

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/9bdf963ecf903382bf1c1e5348fdbcffc792932b
Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r=billm

https://hg.mozilla.org/integration/fx-team/rev/c507aa4e2a21bf01e7580fc431a313b0b442fc61
Bug 1234020: Part 3 - [webext] Remove promise<->callback compatibility workarounds. r=billm

Comment 81

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bdf963ecf90
https://hg.mozilla.org/mozilla-central/rev/c507aa4e2a21