Closed Bug 1293298 Opened 3 years ago Closed 3 years ago

Implement events in SubTypes defined in the WebExtensions API schema files

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Iteration:
56.1 - Jun 26
Tracking Status
firefox56 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(Whiteboard: DevTools API, triaged)

Attachments

(1 file)

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.
Depends on: 1290901
Priority: -- → P3
Whiteboard: DevTools API, triaged
Blocks: 1341304
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P3 → P1
Attachment #8875277 - Flags: review?(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
Attachment #8875277 - Flags: review?(aswan) → review+
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).
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?
Flags: needinfo?(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.
Flags: needinfo?(standard8)
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.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/91c749e9a637
Implement events in SubTypes defined in the WebExtensions API schema files. r=aswan
https://hg.mozilla.org/mozilla-central/rev/91c749e9a637
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.