Support errors for open extension API via lastError

RESOLVED FIXED in Firefox 47

Status

defect
P2
normal
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: billm, Assigned: kmag)

Tracking

(Blocks 1 bug, {dev-doc-complete})

34 Branch
mozilla47
Dependency tree / graph
Bug Flags:
blocking-webextensions -

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [extension])

Attachments

(2 attachments)

Priority: -- → P2
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Blocks: 1213426
Whiteboard: [extension]
Blocks: 1213675
Flags: blocking-webextensions-
Blocks: 1225215
Assignee: nobody → kmaglione+bmo
https://reviewboard.mozilla.org/r/32075/#review28795

::: toolkit/components/extensions/ext-runtime.js:53
(Diff revision 1)
> +                                      Promise.reject({ message: "Invalid extension ID" }));

This change is fairly arbitrary. I just needed a good, simple method to test this on until we start adding it in more places.
Comment on attachment 8711293 [details]
MozReview Request: Bug 1190680: Part 1 - Factor common extension context logic into a shared base class. r?billm

https://reviewboard.mozilla.org/r/32073/#review29547

Looks great. This will help with the OOP stuff.

::: toolkit/components/extensions/Extension.jsm:218
(Diff revision 1)
> -ExtensionPage = function(extension, params) {
> +ExtensionPage = class extends BaseContext {

Why not |class ExtensionPage ...|?

::: toolkit/components/extensions/ExtensionContent.jsm:242
(Diff revision 1)
> -    prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
> +      prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);

I'm not too comfortable with how the |principal| property isn't defined here. Why not just make it be the null principal?
Attachment #8711293 - Flags: review?(wmccloskey) → review+
Attachment #8711294 - Flags: review?(wmccloskey) → review+
Comment on attachment 8711294 [details]
MozReview Request: Bug 1190680: Part 2 - Add initial support for lastError callbacks. r?billm

https://reviewboard.mozilla.org/r/32075/#review29599

::: toolkit/components/extensions/ExtensionUtils.jsm:206
(Diff revision 1)
> +  wrapCallback(callback, promise) {

The name and comment here are not that descriptive. It looks like we're going to be using this to run |callback| when |promise| resolves. I'd be happier with a name like |completePromise| or something, with the callback as the second arg. I don't see how wrapping is the right term here.

Also, the comment should give more of an explanation of how this should be used rather than what it does. Knowing how it's used, I think it should be pretty obvious what it does. Something along the lines of "This function takes a promise representing the completion of an asynchronous API operation and ensures that the extension's callback will run and |lastError| will be set correctly. In the case that no callback is provided, it returns a promise that can be returned to the extension."

::: toolkit/components/extensions/ExtensionUtils.jsm:218
(Diff revision 1)
> +      return new this.cloneScope.Promise((resolve, reject) => {
> +        promise.then(
> +          value => { runSafeSync(this, resolve, value); },
> +          value => {
> +            if (!(value instanceof this.cloneScope.Error)) {
> +              value = new this.cloneScope.Error(value.message);
> +            }
> +            runSafeSyncWithoutClone(reject, value);
> +          });
> +      });
> +    }

It looks like this is a starting point for bug 1234020. Is that the intention?

::: toolkit/components/extensions/Schemas.jsm:80
(Diff revision 1)
> +    throw new global.Error(`${msg} for ${this.namespaceName}.${this.name}.`);

I'm kinda worried about this since most of these subclasses don't have namespaceName or name. It will be easy to forget and use throwError in more places. I'd kinda rather just duplicate the code for now. It's not that much.
https://reviewboard.mozilla.org/r/32073/#review29547

> Why not |class ExtensionPage ...|?

It's used earlier in the module, and we have an ESLint rule that prevents using variables before they're declared.

> I'm not too comfortable with how the |principal| property isn't defined here. Why not just make it be the null principal?

Agree. I'm not sure why I didn't do that.
https://reviewboard.mozilla.org/r/32075/#review29599

> The name and comment here are not that descriptive. It looks like we're going to be using this to run |callback| when |promise| resolves. I'd be happier with a name like |completePromise| or something, with the callback as the second arg. I don't see how wrapping is the right term here.
> 
> Also, the comment should give more of an explanation of how this should be used rather than what it does. Knowing how it's used, I think it should be pretty obvious what it does. Something along the lines of "This function takes a promise representing the completion of an asynchronous API operation and ensures that the extension's callback will run and |lastError| will be set correctly. In the case that no callback is provided, it returns a promise that can be returned to the extension."

I'm not too happy with the name either. I'm leaning toward `wrapPromise`, though, since one of the things it does is wrap privileged promies with promises belonging to the context, which is one of its main uses for bug bug 1234020. That doesn't make it obvious what the callback is for, though. I'll think about it some more.

I'll definitely try to make the docstring clearer.

> It looks like this is a starting point for bug 1234020. Is that the intention?

Yeah, I have some patches for that that depend on this.

> I'm kinda worried about this since most of these subclasses don't have namespaceName or name. It will be easy to forget and use throwError in more places. I'd kinda rather just duplicate the code for now. It's not that much.

Yeah, I guess that's a good point.
https://hg.mozilla.org/integration/fx-team/rev/0573e9dfa2b8fbe1c01426768b1f0edfa2f1153d
Bug 1190680: Part 1 - [webext] Factor common extension context logic into a shared base class. r=billm

https://hg.mozilla.org/integration/fx-team/rev/32faeeebe2e0b4a1c667b16781ac4d0b6cde62e5
Bug 1190680: Part 2 - [webext] Add initial support for lastError callbacks. r=billm
https://hg.mozilla.org/mozilla-central/rev/0573e9dfa2b8
https://hg.mozilla.org/mozilla-central/rev/32faeeebe2e0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1244488
Duplicate of this bug: 1213675
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.