1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Banner (:standard8)]

No-unused-vars is useful as it highlights potentially dead code, or forgotten intentions.

Since a lot of our content code is used in dialogs and doesn't have strict export definitions, I thinking to begin with, lets just turn on the rule for variables in "local" scope. If we want to do "global" later, we can work out the best way of doing it.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

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

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8805908 [details]
Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

https://reviewboard.mozilla.org/r/89512/#review88916

Nice work.

::: browser/base/content/browser-tabsintitlebar.js:141
(Diff revision 1)
>        if (oldSizeMode == "fullscreen") {
>          return;
>        }
>      }
>  
> -    for (let something in this._disallowed) {
> +    for (let something in this._disallowed) { // eslint-disable-line no-unused-vars

Instead of this can you convert this to a Set and then you can just use size >= 0.

::: browser/base/content/test/newtab/head.js:154
(Diff revision 1)
>    */
>  function performOnCell(aIndex, aFn) {
>    return ContentTask.spawn(gWindow.gBrowser.selectedBrowser,
>                             { index: aIndex, fn: aFn.toString() }, function* (args) {
> -    let cell = content.gGrid.cells[args.index];
> +    let cell = content.gGrid.cells[args.index]; // eslint-disable-line no-unused-vars
>      return eval("(" + args.fn + ")(cell)");

I think an alternative here would be:

    return eval(args.fn)(cell)

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8805908 [details]
Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

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

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8805908 [details]
Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

https://reviewboard.mozilla.org/r/89512/#review88916

> Instead of this can you convert this to a Set and then you can just use size >= 0.

I had to use Object.keys().length, as trying to convert to a Set it was complaining it wasn't iterable.

> I think an alternative here would be:
> 
>     return eval(args.fn)(cell)

That's much nicer!

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8805908 [details]
Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

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

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8805908 [details]
Bug 1313998 - Turn on no-unused-vars for local variable scopes in browser/base/content.

https://reviewboard.mozilla.org/r/89512/#review89056

I don't see why the set needs to be iterable when you'd be removing all attemptes to iterate it, but I'll take this anyway.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/101a5b99a05f
Turn on no-unused-vars for local variable scopes in browser/base/content. r=mossop

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/101a5b99a05f