1190680 - Support errors for open extension API via lastError

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

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

https://developer.chrome.com/extensions/extension#property-lastError

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1190680: Part 1 - Factor common extension context logic into a shared base class. r?billm

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1190680: Part 2 - Add initial support for lastError callbacks. r?billm

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

Comment 3

[Kris Maglione [:kmag]]

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 4

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

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?

Comment 5

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

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.

Comment 6

[Kris Maglione [:kmag]]

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.

Comment 7

[Kris Maglione [:kmag]]

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.

Comment 8

[Kris Maglione [:kmag]]

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

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0573e9dfa2b8
https://hg.mozilla.org/mozilla-central/rev/32faeeebe2e0