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

RESOLVED FIXED in Firefox 52

Status

()

Firefox
General
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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.
(Assignee)

Updated

7 months ago
Summary: Turn on no-unused-vars for local variables in browser/base/content → Turn on no-unused-vars for local variable scopes in browser/base/content
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
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)
Attachment #8805908 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

7 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 6

7 months ago
mozreview-review
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.
Attachment #8805908 - Flags: review?(dtownsend) → review+

Comment 7

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/101a5b99a05f
Turn on no-unused-vars for local variable scopes in browser/base/content. r=mossop
https://hg.mozilla.org/mozilla-central/rev/101a5b99a05f
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.