1367780 - Enable eslint on testing/firefox-ui,mozbase,profiles,specialpowers

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Comment 1

[Geoff Brown [:gbrown]]

Attachment: enable eslint on testing/firefox-ui,mozbase,profiles,specialpowers - mechanical updates

Provides eslint coverage for additional testing/ subdirectories:
 - firefox-ui
 - mozbase
 - profiles
 - specialpowers

This first patch has the simple, mechanical changes - white space, bracket placement and such.

Comment 2

[Geoff Brown [:gbrown]]

Attachment: additional changes

:standard8 - Can we do anything other than skip files containing '#'?

I've used 'globals' to handle 'user_pref', but I wonder if there isn't a better strategy...suggestions?

:jmaher - I've added 'return undefined' to SpecialPowersObserver's receiveMessage(). That should not alter its behavior, but I wonder if this is intentional. Also, any other concerns?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f498a47323f5d111d88e76fbf96a952594da1706

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8871315 [details] [diff] [review]
additional changes

Review of attachment 8871315 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +17,4 @@
>  
>  this.SpecialPowersError = function(aMsg) {
>    Error.call(this);
> +  // let {stack} = new Error();

should we just delete this line?

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Joel Maher ( :jmaher) from comment #3)
> > +  // let {stack} = new Error();
> 
> should we just delete this line?

I thought I would leave it in case it might be handy for debugging, but I don't have a strong opinion.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8871315 [details] [diff] [review]
additional changes

Review of attachment 8871315 [details] [diff] [review]:
-----------------------------------------------------------------

::: .eslintignore
@@ +300,5 @@
>  testing/modules/**
>  # octothorpe used for pref file comment causes parsing error
>  testing/mozbase/mozprofile/tests/files/prefs_with_comments.js
> +# uses '#ifdef'
> +testing/specialpowers/content/specialpowers.js

I don't see an ifdef in this file?

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +233,1 @@
>                             .getInterface(Ci.nsIDOMWindowUtils);

nit: please fix the indentation here to re-align the dots like they were before

Comment 6

[Mark Banner (:standard8)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> Created attachment 8871315 [details] [diff] [review]
> additional changes
> 
> :standard8 - Can we do anything other than skip files containing '#'?

Sometimes there are various ways we can - Firefox UI replaced a lot of #ifdefs with AppConstants.

I've just looked at specialpowersAPI.js - the ifdef there is for B2G which is being removed (bug 1306391), additionally, I can't see any reference where that isB2G getter is actually used:

https://dxr.mozilla.org/mozilla-central/search?q=isB2G&redirect=false

all the checks there seem to be working it out for themselves.

So I think given that, we could remove the ifdef from that file, and remove the '*' from the line in the jar.mn to avoid the preprocessing.

> I've used 'globals' to handle 'user_pref', but I wonder if there isn't a
> better strategy...suggestions?

The pref files don't generally worry me too much, but defining it in globals is probably the simplest way for us.

Comment 7

[Mark Banner (:standard8)]

btw, it may not be appropriate for this bug now, but did you know about ./mach eslint --fix ? It saves a bit of time fixing quite a few things automatically.

Comment 8

[Geoff Brown [:gbrown]]

Thanks for the --fix tip. I was aware of it, but haven't been using it much...I'll reconsider that.

Comment 9

[Geoff Brown [:gbrown]]

(In reply to Mark Banner (:standard8) from comment #6)
> So I think given that, we could remove the ifdef from that file, and remove
> the '*' from the line in the jar.mn to avoid the preprocessing.

Oh, great - that seems to work fine, allowing linting of specialpowersAPI.js.

Comment 10

[Geoff Brown [:gbrown]]

(In reply to Mark Banner (:standard8) from comment #5)
> > +# uses '#ifdef'
> > +testing/specialpowers/content/specialpowers.js
> 
> I don't see an ifdef in this file?

Oops, indeed, there is none -- no need for a special exclusion.

Comment 11

[Geoff Brown [:gbrown]]

Attachment: enable eslint on testing/firefox-ui,mozbase,profiles,specialpowers - mechanical updates

As before, but without exclusions for specialpowers.js and specialpowersAPI.js.

Comment 12

[Geoff Brown [:gbrown]]

Attachment: additional changes

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2726c908647
Enable eslint on testing/firefox-ui,mozbase,profiles,specialpowers - mechanical updates; r=Standard8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9e63ef7613
Additional changes for eslint on testing/firefox-ui,mozbase,profiles,specialpowers; r=Standard8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2726c908647
https://hg.mozilla.org/mozilla-central/rev/0a9e63ef7613