Closed Bug 1389265 Opened 2 years ago Closed 2 years ago

contextualIdentities API resolves the promise when an error occurs

Categories

(WebExtensions :: General, defect, P2)

54 Branch
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: robwu, Assigned: jkt)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

When container tabs are disabled, the contextualIdentities API methods resolve values with "false" [1], while I expected it to be rejecting a promise.

Similarly, the .get and .update methods resolve the promise with `null` instead of rejecting a promise when an error occurs (i.e. when the passed argument is invalid).

The documentation does not mention this. E.g. the documentation of browser.contextualIdentities.query [2] is written as if the only valid return value is an array:

A Promise that will be fulfilled with an array of ContextualIdentity objects, each describing a single identity.

A way to solve this bug is to update the documentation for all contextualIdentity methods and state that the return value can be different (null/false).
But I think that the API looks better if error conditions are propagated through a rejected promise instead of a null/false value.


[1] http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/components/extensions/ext-contextualIdentities.js#27-29
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/query
I think if we are going to solve this we should do this before 57.

We are actually removing the resolve false's in: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602

Maybe I should add the rejects there to prevent so much diff noise.

The original idea of null, false was that you could tell the difference between the two responses. Perhaps we don't need to return anything for those null responses.
Assignee: nobody → jkt
Depends on: 1354602
Keywords: dev-doc-needed
Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173156

::: toolkit/components/extensions/ext-contextualIdentities.js:62
(Diff revision 1)
>      let self = {
>        contextualIdentities: {
>          get(cookieStoreId) {
>            let containerId = getContainerForCookieStoreId(cookieStoreId);
>            if (!containerId) {
> -            return Promise.resolve(null);
> +            return Promise.reject();

Please add a useful error message, e.g.

```
throw new ExtensionError("description here");
```

(import ExtensionError from ExtensionUtils at the top of the file).
Attachment #8896766 - Flags: review-
Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173156

> Please add a useful error message, e.g.
> 
> ```
> throw new ExtensionError("description here");
> ```
> 
> (import ExtensionError from ExtensionUtils at the top of the file).

I am just going to reject with an error message instead of throwing as that seems way more common in extension code.
Comment on attachment 8896766 [details]
Bug 1389265 - Change contextual identity web extension APIs to reject instead of returning null.

https://reviewboard.mozilla.org/r/168056/#review173320
Attachment #8896766 - Flags: review?(aswan) → review+
Priority: -- → P2
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4e43c6fdf21
Change contextual identity web extension APIs to reject instead of returning null. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4e43c6fdf21
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Marking dev-doc-complete, see bug 1395659, comment 20.
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(jkt)
QE not required here either. Thanks.
Flags: needinfo?(jkt) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.