1293298 - Implement events in SubTypes defined in the WebExtensions API schema files

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The WebExtensions DevTools API uses custom defined SubTypes to create the `devtools.panels.elements` and `devtools.panels.sources` properties.

Currently only functions are supported in SubTypes, and in Bug 1290901 we are evaluating to just ignore events and properties defined in SubTypes (because they are not used in the initial subset of DevTools API that we are planning to provide), but this needs to be re-evaluated once we decide to support `devtools.panels.sources`/`devtools.panels.elements` properties in the DevTools APIs.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8875277 [details]
Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

https://reviewboard.mozilla.org/r/146678/#review150834

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8875277 [details]
Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146678/diff/1-2/

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8875277 [details]
Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146678/diff/2-3/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8875277 [details]
Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

https://reviewboard.mozilla.org/r/146676/#review153442

::: toolkit/components/extensions/Schemas.jsm:2138
(Diff revisions 2 - 3)
>      };
>    }
>  };
>  
>  // Represents an "event" defined in a schema namespace.
> -Event = class Event extends CallEntry {
> +Event = class Event extends CallEntry { // eslint-disable-line no-native-reassign

This line was marked by eslint as an error, the reason is that `Event` is considered a native global (which is true in a xul browser or web page contexts, but it is not really a native global in a JSM module).

Unfortunately this `// eslint-disable-line no-native-reassign` seems to be the fix which has the smaller impact on the patch (disabling the global using `/* globals Event:false */` doesn't seem to work, while changing the eslint environment also fixes the linting error, e.g. `/* eslint-env node */` but it seems a bit extreme and it can have more side-effects on the rest of the validation on this file).

We don't have a lot of `// eslint-disable-line no-native-reassign` comments around, but we have already a couple of them (http://searchfox.org/mozilla-central/search?q=eslint-disable-line+no-native-reassign&case=false&regexp=false&path=).

A more long term (and better) solution is probably to define an additional environment in the mozilla eslint plugin (e.g. a new custom `"firefox-jsm-module"` eslint environment), but I think that it would be better to evaluate this in a follow up and keep the fix for the linting error on this patch as simple as possible.

(other possible fixes for the above eslint error could be: rename `Event` or move `class Event` before the `SubModuleType` definitions).

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Mark,
how do you think that it would be better to handle the issue with the "no-native-reassign" eslint rule described in Comment 5?

Comment 7

[Mark Banner (:standard8)]

We probably don't have a lot of no-native-reassign comments around as it is only enabled for a few directories currently.

For now, I agree that eslint-disable-line is probably the best way to go. Changing the ESLint set-up would also be good (I've pondered about that a couple of times) and I've just noticed that bug 1369722 got filed for that - I don't have a timescale for it at the moment, the most difficult part would be managing the configuration for targeting just the .jsm files, there's something coming up in ESLint sometime that might help that though.

The slight alternative to eslint-disable-line is eslint-disable-next-line which may be nicer in some cases.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8875277 [details]
Bug 1293298 - Implement events in SubTypes defined in the WebExtensions API schema files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146678/diff/3-4/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks Mark!

I added a reference to Bug 1369722 in an inline comment above the eslint-disable-line, so that we can remember which bugzilla issue to check before trying to remove the eslint-disable-line comment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c749e9a637
Implement events in SubTypes defined in the WebExtensions API schema files. r=aswan

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/91c749e9a637