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

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Comment 1

2 years ago
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)
Assignee

Comment 2

2 years ago
Posted 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+
Assignee

Comment 4

2 years ago
(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.
Assignee

Comment 8

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

Comment 9

2 years ago
(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.
Assignee

Comment 10

2 years ago
(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.
Assignee

Comment 11

2 years ago
As before, but without exclusions for specialpowers.js and specialpowersAPI.js.
Attachment #8871311 - Attachment is obsolete: true
Attachment #8871873 - Flags: review+
Assignee

Comment 12

2 years ago
Attachment #8871315 - Attachment is obsolete: true
Attachment #8871874 - Flags: review?(standard8)
Attachment #8871874 - Flags: review?(standard8) → review+

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2726c908647
https://hg.mozilla.org/mozilla-central/rev/0a9e63ef7613
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

Last year
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.