Closed
Bug 1389265
Opened 8 years ago
Closed 8 years ago
contextualIdentities API resolves the promise when an error occurs
Categories
(WebExtensions :: General, defect, P2)
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
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-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
::: 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-
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-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/#review173320
Attachment #8896766 -
Flags: review?(aswan) → review+
Updated•8 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
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
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 11•8 years ago
|
||
Marking dev-doc-complete, see bug 1395659, comment 20.
Keywords: dev-doc-needed → dev-doc-complete
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
QE not required here either. Thanks.
Flags: needinfo?(jkt) → qe-verify-
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•