1257246 - Support eslint 2

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Dave Townsend [:mossop]]

ESlint 2 made a number of changes that break our plugin and configs. Many other projects are switching and I'm hitting issues with having the wrong version installed

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x and eslint 2.x. r?pbrosset

Since we're already relying on internals it makes more sense to just use
escope's function for declaring variables. We then have to remove references
from the through array and update them to point to their variables, this was
cribbed from https://github.com/eslint/eslint/blob/master/lib/eslint.js#L180

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

Comment 2

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs

For some reason the eslint job isn't picking these up...

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

Comment 3

[Dave Townsend [:mossop]]

We still fail in eslint 2, some rules have gotten better at spotting problems and some rules have been replaced. The latter is hard to fix without either removing the rules or dropping them to warnings which is basically the same thing or dropping support for eslint 1.x. Any thoughts?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #2)
> Created attachment 8731389 [details]
> MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs
> 
> For some reason the eslint job isn't picking these up...
> 
> Review commit: https://reviewboard.mozilla.org/r/40535/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/40535/

Turns out it isn't picking them up because the html plugin we use in automation is older and doesn't detect these issues apparently.

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8731389 [details]
MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs

https://reviewboard.mozilla.org/r/40535/#review37235

::: toolkit/content/plugins.html:144
(Diff revision 1)
>          var stateDd = document.createElement("dd");
>          var state = document.createElement("span");
>          state.setAttribute("label", "state");
>          state.appendChild(document.createTextNode(pluginsbundle.GetStringFromName("state_label") + " "));
>          stateDd.appendChild(state);
> -        status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled");
> +        var status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled");

This is a bug in eslint 2, which we should probably file:

https://developer.mozilla.org/en-US/docs/Web/API/Window/status

as with any property of window, you can access it from anywhere running with that window as the global object without prefixing it with "window". For more such confusing fun, see e.g. https://github.com/mozilla/readability/issues/272 .

rs=me to fix it here anyway, because obviously we don't actually *intend* to assign to window.status... Maybe eslint has a separate warning setting for this? That would be useful, actually...

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Dave Townsend [:mossop] from comment #3)
> We still fail in eslint 2, some rules have gotten better at spotting
> problems and some rules have been replaced. The latter is hard to fix
> without either removing the rules or dropping them to warnings which is
> basically the same thing or dropping support for eslint 1.x. Any thoughts?
I was under the impression that the rules that have been replaced in 2.x have equivalent rules that more or less map one to one with the old rules.
I'm not a fan of changing the levels to warnings because then no one really cares anymore as the builds don't fail and it's just noise in the logs (like, when you do have, say, 1 eslint error on try, then it's lost in hundreds of warnings).
How much work are we talking about for fixing errors related to the replaced rules?

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8731388 [details]
MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x and eslint 2.x. r?pbrosset

https://reviewboard.mozilla.org/r/40533/#review37281

Thanks for fiding a way to fix this with eslint 2.x.
It's unfortunate that eslint still doesn't provide an officially supported way to register new globals from rules. So we still have to do really hacky things to get it working.
Could you please add a comment at the top of the function you changed explaining this? That there's no clean way of doing this, and that this is likely to break in the future if eslint's internals change again.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Patrick Brosset [:pbro] from comment #6)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > We still fail in eslint 2, some rules have gotten better at spotting
> > problems and some rules have been replaced. The latter is hard to fix
> > without either removing the rules or dropping them to warnings which is
> > basically the same thing or dropping support for eslint 1.x. Any thoughts?
> I was under the impression that the rules that have been replaced in 2.x
> have equivalent rules that more or less map one to one with the old rules.
> I'm not a fan of changing the levels to warnings because then no one really
> cares anymore as the builds don't fail and it's just noise in the logs
> (like, when you do have, say, 1 eslint error on try, then it's lost in
> hundreds of warnings).
> How much work are we talking about for fixing errors related to the replaced
> rules?

Switching to the replaced rules is easy, like you say they are pretty much a 1:1 match, but then if you try to use eslint 1.x it throws errors about unknown rules. That's fine but will confuse folks who are still running 1.x. We might want to implement an eslint version check in mach to warn about that.

The harder challenge is the updated rules, consistent-return and generator-star-spacing are flagging a lot more errors than previously, the former is now spotting cases where a function returns something in some cases but then runs to completion without returning in others. That's a valid issue and unfortunately requires manual work. I haven't looked into why the other is flagging so much yet but at least that is automatically fixable.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Patrick Brosset [:pbro] from comment #7)
> Comment on attachment 8731388 [details]
> MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x
> and eslint 2.x. r?pbrosset
> 
> https://reviewboard.mozilla.org/r/40533/#review37281
> 
> Thanks for fiding a way to fix this with eslint 2.x.
> It's unfortunate that eslint still doesn't provide an officially supported
> way to register new globals from rules. So we still have to do really hacky
> things to get it working.
> Could you please add a comment at the top of the function you changed
> explaining this? That there's no clean way of doing this, and that this is
> likely to break in the future if eslint's internals change again.

One idea that occurred to me is that rather than using a rule for this we could use a custom JS parser based on espree. That would let us change the "Cu.import" AST nodes into "var foo" nodes before eslint even sees them. That would probably give us better insulation from eslint internal changes.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Patrick Brosset [:pbro] from comment #7)
> Could you please add a comment at the top of the function you changed
> explaining this? That there's no clean way of doing this, and that this is
> likely to break in the future if eslint's internals change again.

That comment already exists from when Mike first wrote the original code.(In reply to :Gijs Kruitbosch from comment #5)

> Comment on attachment 8731389 [details]
> MozReview Request: Bug 1257246: Fix some failures in html tests. r?gijs
> 
> https://reviewboard.mozilla.org/r/40535/#review37235
> 
> ::: toolkit/content/plugins.html:144
> (Diff revision 1)
> >          var stateDd = document.createElement("dd");
> >          var state = document.createElement("span");
> >          state.setAttribute("label", "state");
> >          state.appendChild(document.createTextNode(pluginsbundle.GetStringFromName("state_label") + " "));
> >          stateDd.appendChild(state);
> > -        status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled");
> > +        var status = plugin.isActive ? pluginsbundle.GetStringFromName("state_enabled") : pluginsbundle.GetStringFromName("state_disabled");
> 
> This is a bug in eslint 2, which we should probably file:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Window/status

https://github.com/sindresorhus/globals/pull/79

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9a7f99aa5607
https://hg.mozilla.org/integration/fx-team/rev/090b7c54cbb2

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Dave Townsend [:mossop] from comment #9)
> (In reply to Patrick Brosset [:pbro] from comment #7)
> > Comment on attachment 8731388 [details]
> > MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x
> > and eslint 2.x. r?pbrosset
> > 
> > https://reviewboard.mozilla.org/r/40533/#review37281
> > 
> > Thanks for fiding a way to fix this with eslint 2.x.
> > It's unfortunate that eslint still doesn't provide an officially supported
> > way to register new globals from rules. So we still have to do really hacky
> > things to get it working.
> > Could you please add a comment at the top of the function you changed
> > explaining this? That there's no clean way of doing this, and that this is
> > likely to break in the future if eslint's internals change again.
> 
> One idea that occurred to me is that rather than using a rule for this we
> could use a custom JS parser based on espree. That would let us change the
> "Cu.import" AST nodes into "var foo" nodes before eslint even sees them.
> That would probably give us better insulation from eslint internal changes.
That would be pretty cool! We'd have to be careful with line numbers though, but that seems like a small problem. Could eslint pre-processors (I forget what they are called) be used for this?

Comment 13

[Dave Townsend [:mossop]]

(In reply to Patrick Brosset [:pbro] from comment #12)
> (In reply to Dave Townsend [:mossop] from comment #9)
> > (In reply to Patrick Brosset [:pbro] from comment #7)
> > > Comment on attachment 8731388 [details]
> > > MozReview Request: Bug 1257246: Fix addVarToScope to work in both eslint 1.x
> > > and eslint 2.x. r?pbrosset
> > > 
> > > https://reviewboard.mozilla.org/r/40533/#review37281
> > > 
> > > Thanks for fiding a way to fix this with eslint 2.x.
> > > It's unfortunate that eslint still doesn't provide an officially supported
> > > way to register new globals from rules. So we still have to do really hacky
> > > things to get it working.
> > > Could you please add a comment at the top of the function you changed
> > > explaining this? That there's no clean way of doing this, and that this is
> > > likely to break in the future if eslint's internals change again.
> > 
> > One idea that occurred to me is that rather than using a rule for this we
> > could use a custom JS parser based on espree. That would let us change the
> > "Cu.import" AST nodes into "var foo" nodes before eslint even sees them.
> > That would probably give us better insulation from eslint internal changes.
> That would be pretty cool! We'd have to be careful with line numbers though,
> but that seems like a small problem. Could eslint pre-processors (I forget
> what they are called) be used for this?

No need. You can override the parser in the eslintrc config (many folks use babel-eslint as a parser to support more modern JS than espree does). Each node includes it line number in it I think so we'd just make declaration nodes with the same lines as the Cu.import nodes I guess. A little harder when the Cu.import is somewhere within a function but still declaring globals, same goes for the comment directives we use to add globals so there is a bit of work there to get it right otherwise I might have tried already! Worth a thought though.

Comment 14

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal

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

Comment 15

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

This imports semantic_version from https://github.com/rbarrois/python-semanticversion/tree/v2.5.0
so we can verify the version of installed npm modules against a semantic version
requirement.

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

Comment 16

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs

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

Comment 17

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update devtools for eslint2.

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

Comment 18

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Various fixes for linting failures

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

Comment 19

[Dave Townsend [:mossop]]

Apparently with eslint 2 generator-star-spacing started checking anonymous functions as well as regular ones, that is why so many new errors appeared.

I put a bunch of updates into mozreview including switching versions in mach eslint (which pre-run version checks!) and the lint-on-checking task and updating a bunch of the fixes. I'm not likely to have much time to do more at the moment though and we can't land this piecemeal. If anyone feels like fixing more of the failures please feel free to take over!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a7f99aa5607
https://hg.mozilla.org/mozilla-central/rev/090b7c54cbb2

Comment 21

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka

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

Comment 22

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN

For now this means turning off keyword-spacing. This replaced the old
space-before-keywords and space-after-keywords but for some reason now notices
that our spacing after some keywords, particularly catch, switch and while is
very inconsistent. Correcting one way or another is time consuming because
eslint's auto-fixing can't work on html or xbl files.

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

Comment 23

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan

Most of this is fixing functions that in some cases return a value but then
can also run to completion without returning anything. ESLint 2 catches this
where previous versions didn't. Unless there was an obvious other choice I just
made these functions return undefined at the end which is effectively what
already happens.

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

Comment 24

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe

Most of this is fixing functions that in some cases return a value but then
can also run to completion without returning anything. ESLint 2 catches this
where previous versions didn't. Unless there was an obvious other choice I just
made these functions return undefined at the end which is effectively what
already happens.

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

Comment 25

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag

ESLint 2 now flags anonymous generator functions that don't match the
generator-star-spacing rule so this mostly is fixing that.

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

Comment 26

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro

ESLint 2 now flags anonymous generator functions according to
generator-star-spacing. Most of the changes here are correcting that.

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

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 8731821 [details]
MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

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

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 8731820 [details]
MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal

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

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 8731822 [details]
MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs

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

Comment 30

[Dave Townsend [:mossop]]

With the latest set of patches everything passes eslint 2 except for two devtools files where it is flagging an error on functions that are too complex. Could you find someone to fix those Patrick?

Comment 31

[:Gijs (out for now; he/him)]

Comment on attachment 8731822 [details]
MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs

https://reviewboard.mozilla.org/r/40859/#review41323

::: toolkit/components/aboutmemory/content/aboutMemory.js:1171
(Diff revision 2)
>        case UNITS_COUNT_CUMULATIVE: return formatInt(this._amount);
>        case UNITS_PERCENTAGE:       return formatPercentage(this._amount);
>        default:
>          assertInput(false, "bad units in TreeNode.toString");
>      }
> +    return undefined;

The default case in the switch statement actually throws indirectly.

Can you just do that directly instead to shut this up?

throw new Error("bad units in TreeNode.toString");

will also get us a stack here if anyone ever hits this.

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm:224
(Diff revision 2)
>  
>  function debug(msg, error=null) {
>    if (DEBUG_LOG) {
>      return log(msg, "DEBUG: ", error);
>    }
> +  return undefined;

log() has no return value. Just stop returning its rv here and below.

::: toolkit/components/ctypes/tests/unit/head.js:77
(Diff revision 2)
>      bsource = b.toSource();
>      finished = true;
>    } catch (x) {
>    }
>    if (finished) {
>      return do_check_eq(asource, bsource);

do_check_eq throws, so you can just replace this with:

do_check_eq(asource, bsource);
return;

which will avoid having to return undefined;

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:653
(Diff revision 2)
>        let probe = Probes[probeName];
>        if (!probe.isEqual(this[probeName], to[probeName])) {
>          return false;
>        }
>      }
> +    return undefined;

Um. Yeah. Maybe return true? It looks like that's what it's supposed to do and it accidentally got removed in https://hg.mozilla.org/mozilla-central/rev/ca96c76db6a2 .

::: toolkit/content/tests/browser/browser_findbar.js:159
(Diff revision 2)
>  
>    yield promiseFindFinished("s", false);
>    ok(!findbar._findStatusDesc.textContent, "Findbar status should be empty");
>  
>    yield BrowserTestUtils.removeTab(tab);
> +  return undefined;

Please change the earlier 'return true' to just 'return'

::: toolkit/content/tests/browser/browser_findbar.js:199
(Diff revision 2)
>    is(document.activeElement, findBar._findField.inputField,
>      "findbar is now focused");
>    is(findBar._findField.value, "abc", "abc fully entered as find query");
>  
>    yield BrowserTestUtils.removeTab(tab);
> +  return undefined;

Ditto.

::: toolkit/content/tests/browser/head.js:24
(Diff revision 2)
>        }
>        findbar.removeEventListener("transitionend", cont);
>        resolve();
>      });
>      findbar.close();
> +    return undefined;

Please brace the first if statement and stick `resolve()` and `return` on separate lines.

::: toolkit/modules/Log.jsm:371
(Diff revision 2)
>        level = this.level;
>      }
>  
>      params.action = action;
>      this.log(level, params._message, params);
> +    return undefined;

Stick this.log() on a separate line from the early return.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8738620 [details]
MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN

https://reviewboard.mozilla.org/r/44615/#review41357

I think there's something missing in this patch as all I see changing in .eslintrc is commented code which doesn't match the commit message.

Comment 33

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/44615/#review41357

Oh you're right, I didn't notice that the deprecated rules were already commented out. No wonder I saw so many errors trying to turn on keyword-spacing!. So basically all this patch does is remove the commented out old ESLint 1 rules and replace them with their replacement still commented out. Also turns off linting for microformat tests which are external code.

Comment 34

[Andrew Halberstadt [:ahal]]

Comment on attachment 8731820 [details]
MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal

https://reviewboard.mozilla.org/r/40855/#review41363

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8738620 [details]
MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN

https://reviewboard.mozilla.org/r/44615/#review41365

Comment 36

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/44615/#review41367

::: toolkit/.eslintrc:46
(Diff revision 1)
> +    // Require spaces before and after keywords
> +    // "keyword-spacing": 2,
> +

Please update the commit message to be more accurate now.

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8738623 [details]
MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag

https://reviewboard.mozilla.org/r/44621/#review41375

::: browser/components/extensions/test/browser/file_popup_api_injection_a.html:6
(Diff revision 1)
>  <!DOCTYPE html>
>  <html>
>  <head>
>    <meta charset="utf-8">
>    <script type="application/javascript">
> +    /* eslint strict: "off" */

I'd rather we just add "use strict"

::: browser/components/extensions/test/browser/file_popup_api_injection_b.html:6
(Diff revision 1)
>  <!DOCTYPE html>
>  <html>
>  <head>
>    <meta charset="utf-8">
>    <script type="application/javascript">
> +    /* eslint strict: "off" */

Same here.

::: browser/components/extensions/test/browser/head.js
(Diff revision 1)
>    if (group.areaType == CustomizableUI.TYPE_TOOLBAR) {
>      return win.document.getElementById("customizationui-widget-panel");
>    } else {
>      return win.PanelUI.panel;
>    }
> -  return null;

Let's also get rid of the `else`

::: toolkit/components/extensions/.eslintrc:343
(Diff revision 1)
>  
>      // Allow comments inline after code.
>      "no-inline-comments": 0,
>  
> +    // Disallow use of labels for anything other then loops and switches.
> +    "no-labels": 2,

This should have `{"allowLoop": true}`

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8738622 [details]
MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe

https://reviewboard.mozilla.org/r/44619/#review41371

::: browser/base/content/browser-plugins.js
(Diff revision 1)
>    MESSAGES: [
>      "PluginContent:ShowClickToPlayNotification",
>      "PluginContent:RemoveNotification",
>      "PluginContent:UpdateHiddenPluginUI",
>      "PluginContent:HideNotificationBar",
> -    "PluginContent:ShowInstallNotification",

is this part of another patch? or are you removing something that was unused to simplify the fix for receiveMessage below?

::: browser/base/content/browser.js:3352
(Diff revision 1)
>  
>        case "Link:AddSearch":
>          this.addSearch(aMsg.target, aMsg.data.engine, aMsg.data.url);
>          break;
>      }
> +    return undefined;

why is this one necessary? I don't see anything else returning on this function

::: browser/base/content/sync/setup.js:124
(Diff revision 1)
>    startNewAccountSetup: function () {
>      if (!Weave.Utils.ensureMPUnlocked())
>        return false;
>      this._settingUpNew = true;
>      this.wizard.pageIndex = NEW_ACCOUNT_START_PAGE;
> +    return undefined;

here (and the function below), I believe it's better to just replace the early `return false;` with just `return;`.

These two are called from oncommand="foo()" handlers. Do you know if there are differences in behavior from a function returning false and not returning?

::: browser/components/customizableui/CustomizableUI.jsm:2137
(Diff revision 1)
>      aWindow.gNavToolbox.dispatchEvent(evt);
>    },
>  
>    dispatchToolboxEvent: function(aEventType, aDetails={}, aWindow=null) {
>      if (aWindow) {
>        return this._dispatchToolboxEventToWindow(aEventType, aDetails, aWindow);

`this._dispatchToolboxEventToWindow` doesn't return anything, so it's better to just split these line in two to put this early return as a standalone line.

Comment 39

[:Cykesiopka]

Comment on attachment 8738619 [details]
MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka

https://reviewboard.mozilla.org/r/44613/#review41441

LGTM. Thanks for doing this!

Comment 40

[Andrew Swan [:aswan]]

Comment on attachment 8738621 [details]
MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan

https://reviewboard.mozilla.org/r/44617/#review41443

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:668
(Diff revision 1)
>        }, aSendPerformance, aTimeout));
>  
>      // Always call AddonManager updateAddonRepositoryData after we refill the cache
>      yield new Promise((resolve, reject) =>
>        AddonManagerPrivate.updateAddonRepositoryData(resolve));
> +    return undefined;

If I read this right, it looks like the `return this._clearCache();` lines in the body above could be changed to `yield this._clearCache(); return;` and that might be clearer?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3057
(Diff revision 1)
>          }
>        }
>  
>        yield systemAddonLocation.cleanDirectories();
>      }
> +    return undefined;

basically same as above, if we change `return systemAddonLocation.cleanDirectories();` to `yield ...; return;` it seems to be easier to read and understand.

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8738624 [details]
MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro

https://reviewboard.mozilla.org/r/44623/#review41497

::: devtools/client/animationinspector/test/head.js:217
(Diff revision 1)
> -var getAnimationPlayerState = Task.async(function*(selector,
> +var getAnimationPlayerState = Task.async(function* (selector,
>                                                     animationIndex = 0) {

`animationIndex = 0` on the second line was supposed to align vertically with `selector`. Now that there is an extra space, it doesn't anymore.
Can you insert a new space on the second line?

::: devtools/client/animationinspector/test/head.js:268
(Diff revision 1)
> -    timeline.once("timeline-data-changed", () => hasMoved = true);
> +    timeline.once("timeline-data-changed", () => {
> +      hasMoved = true;
> +    });

Looks like rule no-return-assign in eslint 2 now also checks arrow functions.
Honestly, I don't know if this is what we want. Arrow functions are very often used (like here) as a very compact way to define behavior. And having to wrap this on 3 lines is too bad.

`() => hasMoved = true` is short and sweet, and fits nicely in a one liner. This is the beauty of arrow functions.

So, either we disable this rule for devtools, or we do this one change (I haven't yet reviewed everything, but I doubt that there are many similar fixes in this commit).

Ok, disabling the rule has way more impact than keeping these few fixes, so forget about this. We can revisit this rule in devtools later if it frustrate people.

::: devtools/client/inspector/markup/test/browser_markup_keybindings_delete_attributes.js:10
(Diff revision 1)
>  "use strict";
>  
>  // Tests that attributes can be deleted from the markup-view with the delete key
>  // when they are focused.
>  
> -const HTML = `<div id="id" class="class" data-id="id"></div>`;
> +const HTML = "<div id=\"id\" class=\"class\" data-id=\"id\"></div>";

We have the "avoid-escape" especially for this, so you could simply replace the backticks with single quotes instead:

```
const HTML = '<div id="id" class="class" data-id="id"></div>';
```

::: devtools/client/inspector/rules/test/head.js:662
(Diff revision 1)
> -var addProperty = Task.async(function*(view, ruleIndex, name, value,
> +var addProperty = Task.async(function* (view, ruleIndex, name, value,
>                                         commitValueWith = "VK_RETURN",
>                                         blurNewProperty = true) {

Same comment about vertically aligning these arguments.
Please insert a space on the 2 lines that wrap below the function definition.

::: devtools/client/inspector/rules/test/head.js:720
(Diff revision 1)
> -var setProperty = Task.async(function*(view, textProp, value,
> +var setProperty = Task.async(function* (view, textProp, value,
>                                         blurNewProperty = true) {

ditto

::: devtools/client/inspector/rules/test/head.js:753
(Diff revision 1)
> -var removeProperty = Task.async(function*(view, textProp,
> +var removeProperty = Task.async(function* (view, textProp,
>                                            blurNewProperty = true) {

ditto

Now that I'm done reviewing, I counted 6 places where you had to wrap an arrow function on 3 lines because of the no-return-assign rule change. Knowing the amount of code in devtools, this is a small number. So let's keep it as you did.

Comment 42

[Patrick Brosset <:pbro>]

(In reply to Dave Townsend [:mossop] from comment #30)
> With the latest set of patches everything passes eslint 2 except for two
> devtools files where it is flagging an error on functions that are too
> complex. Could you find someone to fix those Patrick?
Oh I see, we have configured the complexity rule as an error in our .eslintrc config file, but we have never configured a complexity threshold.
And, according to https://github.com/eslint/eslint/issues/4808, doing this has no effect. So essentially, this rule was off for us.
Now, since this change: https://github.com/eslint/eslint/commit/1288ba435d6758394bf97495583159b32742bfaa the default value is 20.
And it looks like some of our functions are above this limit. Running eslint 2.7.0 locally produced the following results for me:

\devtools\client\inspector\markup\markup.js
  606:15  error  Function '_onKeyDown' has a complexity of 32   complexity

\devtools\client\netmonitor\netmonitor-view.js
  1112:11  error  Function 'sortBy' has a complexity of 21          complexity
  1498:19  error  Function '_flushRequests' has a complexity of 35  complexity
  1781:30  error  Function 'anonymous' has a complexity of 26       complexity

So, because it was off until now, I would suggest changing our .eslintrc instead of fixing these functions (at least for now). This should do it:

"complexity": [2, 35]

I'll file a bug to reduce the threshold to 20 again and fix the functions.

Comment 43

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/44619/#review41371

> is this part of another patch? or are you removing something that was unused to simplify the fix for receiveMessage below?

Yeah this message was removed when the plugin-finder was dropped but this bit was never removed and the bit below made eslint fail.

Comment 44

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/44623/#review41497

> We have the "avoid-escape" especially for this, so you could simply replace the backticks with single quotes instead:
> 
> ```
> const HTML = '<div id="id" class="class" data-id="id"></div>';
> ```

Huh, looks like quotes considers using a single quote in that case as ok but not using backticks.

Comment 45

[Dave Townsend [:mossop]]

Comment on attachment 8731821 [details]
MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

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

Comment 46

[Dave Townsend [:mossop]]

Comment on attachment 8731820 [details]
MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal

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

Comment 47

[Dave Townsend [:mossop]]

Comment on attachment 8738619 [details]
MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka

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

Comment 48

[Dave Townsend [:mossop]]

Comment on attachment 8738620 [details]
MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN

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

Comment 49

[Dave Townsend [:mossop]]

Comment on attachment 8738621 [details]
MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan

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

Comment 50

[Dave Townsend [:mossop]]

Comment on attachment 8738622 [details]
MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe

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

Comment 51

[Dave Townsend [:mossop]]

Comment on attachment 8731822 [details]
MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs

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

Comment 52

[Dave Townsend [:mossop]]

Comment on attachment 8738623 [details]
MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag

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

Comment 53

[Dave Townsend [:mossop]]

Comment on attachment 8738624 [details]
MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro

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

Comment 54

[Gregory Szorc [:gps]]

Comment on attachment 8731821 [details]
MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

https://reviewboard.mozilla.org/r/40857/#review41735

I'm going to push back on adding semantic_version to the tree. Can we use LooseVersion from distutils (part of the standard library) instead? https://stackoverflow.com/questions/11887762/how-to-compare-version-style-strings

Comment 55

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/40857/#review41735

I'm going to move the version checking stuff out to bug 1263211

Comment 56

[Dave Townsend [:mossop]]

Comment on attachment 8731821 [details]
MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

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

Comment 57

[Dave Townsend [:mossop]]

Comment on attachment 8731820 [details]
MozReview Request: Bug 1257246: Update lint test image to newer packages of eslint. r?ahal

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

Comment 58

[Dave Townsend [:mossop]]

Comment on attachment 8738619 [details]
MozReview Request: Bug 1257246: Update security/manager for eslint 2. r?cykesiopka

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

Comment 59

[Dave Townsend [:mossop]]

Comment on attachment 8738620 [details]
MozReview Request: Bug 1257246: Update eslint rules for eslint 2. r?MattN

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

Comment 60

[Dave Townsend [:mossop]]

Comment on attachment 8738621 [details]
MozReview Request: Bug 1257246: Update toolkit/mozapps/extensions for eslint 2. r?aswan

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

Comment 61

[Dave Townsend [:mossop]]

Comment on attachment 8738622 [details]
MozReview Request: Bug 1257246: Update browser for eslint 2. r?felipe

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

Comment 62

[Dave Townsend [:mossop]]

Comment on attachment 8731822 [details]
MozReview Request: Bug 1257246: Update toolkit for eslint 2. r?Gijs

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

Comment 63

[Dave Townsend [:mossop]]

Comment on attachment 8738623 [details]
MozReview Request: Bug 1257246: Update webextension APIs for eslint 2. r?kmag

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

Comment 64

[Dave Townsend [:mossop]]

Comment on attachment 8738624 [details]
MozReview Request: Bug 1257246: Update devtools for eslint 2. r?pbro

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

Comment 65

[Gregory Szorc [:gps]]

Comment on attachment 8731821 [details]
MozReview Request: Bug 1257246: Update the version of eslint that mach installs. r?gps

https://reviewboard.mozilla.org/r/40857/#review41783

Looks good.

+1 for pinning versions. I love more determinism!

Comment 66

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2b2647fba6ce
https://hg.mozilla.org/integration/fx-team/rev/6cfdab897a8f
https://hg.mozilla.org/integration/fx-team/rev/5bb356209841
https://hg.mozilla.org/integration/fx-team/rev/09680d0b42ac
https://hg.mozilla.org/integration/fx-team/rev/4a108098012b
https://hg.mozilla.org/integration/fx-team/rev/5e40adeb0332
https://hg.mozilla.org/integration/fx-team/rev/db2d6e47a3f4
https://hg.mozilla.org/integration/fx-team/rev/bf854efd8e65
https://hg.mozilla.org/integration/fx-team/rev/b332ba21d743

Comment 67

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

https://hg.mozilla.org/mozilla-central/rev/2b2647fba6ce
https://hg.mozilla.org/mozilla-central/rev/6cfdab897a8f
https://hg.mozilla.org/mozilla-central/rev/5bb356209841
https://hg.mozilla.org/mozilla-central/rev/09680d0b42ac
https://hg.mozilla.org/mozilla-central/rev/4a108098012b
https://hg.mozilla.org/mozilla-central/rev/5e40adeb0332
https://hg.mozilla.org/mozilla-central/rev/db2d6e47a3f4
https://hg.mozilla.org/mozilla-central/rev/bf854efd8e65
https://hg.mozilla.org/mozilla-central/rev/b332ba21d743