Support errors for open extension API via lastError

RESOLVED FIXED in Firefox 47

Status

P2
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: billm, Assigned: kmag)

Tracking

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

34 Branch
mozilla47
dev-doc-complete
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
Keywords: dev-doc-needed

Updated

3 years ago
Blocks: 1213426
Whiteboard: [extension]
Blocks: 1213675

Updated

3 years ago
Flags: blocking-webextensions-
Blocks: 1220154
(Assignee)

Updated

3 years ago
Blocks: 1225215
(Assignee)

Updated

3 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Comment 1

3 years ago
Created attachment 8711293 [details]
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/
Attachment #8711293 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

3 years ago
Created attachment 8711294 [details]
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/
Attachment #8711294 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0573e9dfa2b8
https://hg.mozilla.org/mozilla-central/rev/32faeeebe2e0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1244488
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1213675

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.