1311315 - General small fixes for no-undef eslint issues

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Banner (:standard8)]

I've scanned through a lot of the output from:

./mach lint -l eslint browser/ --rule="{no-undef:2}"

and there's simple things we can fix. The patch I'll attach fixes most of those, and as a result cuts down the number of errors by about 478 (8271 -> 7793 on my checkout).

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

Ensures eslint files themselves aren't reported as errors (they are loaded in the 'node' environment but we don't want that for the browser).
Fix some minor errors in the code (missing let/const, undefined vars).
Let eslint know that some files get their globals from other places (typically via xul script tags).

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8802467 [details]
Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

https://reviewboard.mozilla.org/r/86868/#review86066

::: browser/.eslintrc.js:3
(Diff revision 1)
>  "use strict";
>  
> -module.exports = {
> +module.exports = { // eslint-disable-line no-undef

This doesn't seem to scale well.

At the top of .eslintignore we specifically opt-in to having eslint run on the .eslintrc.js files.

Is this something that we should continue doing? I don't think linting our eslint files and having to manually exclude various rules will scale well or provide much benefit. If there is an issue in the eslint file that someone introduces while working on it they will likely notice it right away since that is what they are working on and focusing on.

::: browser/components/customizableui/CustomizableWidgets.jsm:1262
(Diff revision 1)
>    if (Services.appinfo.browserTabsRemoteAutostart) {
>      CustomizableWidgets.push({
>        id: "e10s-button",
>        defaultArea: CustomizableUI.AREA_PANEL,
>        onBuild: function(aDocument) {
> +        let node = aDocument.createElementNS(kNSXUL, "toolbarbutton");

Did this even work before? Yikes!

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8802467 [details]
Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

https://reviewboard.mozilla.org/r/86868/#review86066

> This doesn't seem to scale well.
> 
> At the top of .eslintignore we specifically opt-in to having eslint run on the .eslintrc.js files.
> 
> Is this something that we should continue doing? I don't think linting our eslint files and having to manually exclude various rules will scale well or provide much benefit. If there is an issue in the eslint file that someone introduces while working on it they will likely notice it right away since that is what they are working on and focusing on.

Yeah, :Gijs raised this as well. I was mainly thinking its better for editing (as the editor would pick it up as well), however, having the exclusions are going to be more verbose, so maybe its just best to leave it for now - I'll drop those changes out.

> Did this even work before? Yikes!

It wouldn't have, but it is in a block that's only enabled when E10S_TESTING_ONLY is somehow set. Hence, it isn't a user-facing bug.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8802467 [details]
Bug 1311315 - General small fixes for no-undef eslint issues in browser/.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82e347fa3582
General small fixes for no-undef eslint issues in browser/. r=jaws

Comment 6

[Guilherme Lima]

Landed 22 hours ago https://hg.mozilla.org/mozilla-central/rev/82e347fa3582