Closed Bug 1239438 Opened 4 years ago Closed 4 years ago

fix eslint errors in styles.js

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: tromey, Assigned: tromey)

Details

Attachments

(1 file, 1 obsolete file)

I happened to look at styles.js today and was puzzled by an eslint
error.  This turned into bug 1239426.

Once I fixed that I went ahead and fixed all the easy eslint reports
in styles.js while I was there.

After the patch I'll attach, there are still two reports:

  1018:5  error  "PageStyleFront" is defined but never used  no-unused-vars
  1657:5  error  "StyleRuleFront" is defined but never used  no-unused-vars

Perhaps these should just not be given names.
Comment on attachment 8707597 [details] [diff] [review]
fix most eslint warnings in styles.js

Mostly just basic stuff.

I wasn't sure the best way to deal with the unused function arguments,
so I chose to document the parameters as they are passed -- this code
defining the interface used by the rule rewriter.

Also, the front code, like this:

var StyleRuleFront = protocol.FrontClass(StyleRuleActor, {

... causes a warning, because StyleRuleFront is never referenced.

One idea would be to remove the declaration, so:

protocol.FrontClass(StyleRuleActor, {

I felt this might be less clear; but on the other hand I suppose I could
mention StyleRuleFront in a comment and thus remove the last two warnings.
What do you think?
Attachment #8707597 - Flags: review?(pbrosset)
(In reply to Tom Tromey :tromey from comment #2)
> Also, the front code, like this:
> 
> var StyleRuleFront = protocol.FrontClass(StyleRuleActor, {
> 
> ... causes a warning, because StyleRuleFront is never referenced.
> 
> One idea would be to remove the declaration, so:
> 
> protocol.FrontClass(StyleRuleActor, {
> 
> I felt this might be less clear; but on the other hand I suppose I could
> mention StyleRuleFront in a comment and thus remove the last two warnings.
> What do you think?
Some fronts use exports.MyFront = MyFront, but that's because they are top-level fronts that need to be instantiated by the UI (e.g. ReflowFront in the layout-view).
But in this particular case, the front doesn't need to be instantiated since one of the parent actors creates StyleRuleActor instances and sends this as part of response packets.

If you think it's clearer to keep this var declaration anyway, I see 2 options:
use exports.StyleRuleFront = StyleRuleFront even if no one uses this exported symbol (after all, we might use it from tests at some stage).
or use: /* exported StyleRuleFront */ which tells eslint not to warn about this. But that means we have to manually maintain this comment.

Personally, I'm ok to remove the var declaration.
Attachment #8707597 - Flags: review?(pbrosset) → review+
Updated to remove the declarations.
Attachment #8707597 - Attachment is obsolete: true
Attachment #8707615 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/546b7fdcb156
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.