1328338 - Fix more no-undef eslint issues in toolkit/ and browser/

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Mark Banner (:standard8)]

This is the continuing mission towards enabling the no-undef eslint rule: to explore strange old code, to seek out new bugs and new issues, to boldly fix where no one has fixed before.

Comment 1

[Mark Banner (:standard8)]

The patch I'm about to attach is another mixed lot of fixes, which reduce the errors over the whole tree by about 650, with about 5800 remaining.

Most of those should be dealt with by bug 1323167, which I'll be thinking more about next.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

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

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

https://reviewboard.mozilla.org/r/101914/#review102328

::: toolkit/.eslintrc.js:210
(Diff revision 1)
>    "env": {
>      "es6": true,
>      "browser": true,
>    },
>    "globals": {
> +    "ChromeWorker": false,

Get rid of the extra space indent while you're here please.

::: toolkit/components/crashes/tests/xpcshell/test_crash_store.js:12
(Diff revision 1)
>  
>  "use strict";
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +/* global CrashManager */

How come the imports plugin doesn't detect this?

::: toolkit/components/downloads/test/unit/test_app_rep_windows.js
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>                                    "resource://gre/modules/Promise.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "Task",
> -                                  "resource://gre/modules/Task.jsm");

Why is this removed?

::: toolkit/components/formautofill/test/loader_common.js:15
(Diff revision 1)
>   *
>   * Unless you are adding new features to the framework, you shouldn't have to
>   * modify this file.  Use "head_common.js" or the "head.js" file of each
>   * framework for shared code.
> + *
> + * This file expers Services & XPCOMUtils to be defined in the scope it is imported

s/expers/expects/

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

https://reviewboard.mozilla.org/r/101914/#review102336

Looks like you're failing xpcshell tests too

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

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

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

https://reviewboard.mozilla.org/r/101914/#review102328

> Get rid of the extra space indent while you're here please.

I don't see an extra indent, am I missing something?

> How come the imports plugin doesn't detect this?

I've only just realised what's happening here:

```
var bsp = Cu.import("resource://gre/modules/CrashManager.jsm", this);
```

I'd originally read this line as importing CrashManager into bsp and not into the global scope. However, I've just spotted the `this`, so confusingly, it is importing into bsp *and* the global scope.

The import plugin doesn't detect this, as it is expecting `Cu.import` starting at the beginning of the line only, which I think is generally correct. It feels like we should probably ban this format (which IMHO is potentially confusing), or update the importer to cope with it. In any case, I think this is getting to a separate bug territory and so I'll drop the changes which were attempting to badly handle this from this patch.

> Why is this removed?

I moved it to the head_download_manager.js file.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

https://reviewboard.mozilla.org/r/101914/#review102328

> I don't see an extra indent, am I missing something?

Weird, in the diff view it looked like there was an extra space in there. Never mind.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8823378 [details]
Bug 1328338 - Fix more no-undef eslint issues in toolkit/ and browser/.

https://reviewboard.mozilla.org/r/101914/#review102614

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1078b70896e
Fix more no-undef eslint issues in toolkit/ and browser/. r=mossop

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d1078b70896e