Closed Bug 1367780 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
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.
Attachment #8871311 - Flags: review?(standard8)
Attached patch additional changes (obsolete) — Splinter Review
: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
Attachment #8871315 - Flags: review?(standard8)
Attachment #8871315 - Flags: feedback?(jmaher)
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?
Attachment #8871315 - Flags: feedback?(jmaher) → feedback+
(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.
Attachment #8871311 - Flags: review?(standard8) → review+
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
Attachment #8871315 - Flags: review?(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.
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.
Thanks for the --fix tip. I was aware of it, but haven't been using it much...I'll reconsider that.
(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.
(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.
As before, but without exclusions for specialpowers.js and specialpowersAPI.js.
Attachment #8871311 - Attachment is obsolete: true
Attachment #8871873 - Flags: review+
Attachment #8871315 - Attachment is obsolete: true
Attachment #8871874 - Flags: review?(standard8)
Attachment #8871874 - Flags: review?(standard8) → review+
Pushed by gbrown@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c2726c908647
https://hg.mozilla.org/mozilla-central/rev/0a9e63ef7613
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.