Enable object-shorthand eslint rules for toolkit

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
General
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 9 obsolete attachments)

59 bytes, text/x-review-board-request
MattN
: review+
Details | Review
59 bytes, text/x-review-board-request
MattN
: review+
Details | Review
59 bytes, text/x-review-board-request
MattN
: review+
Details | Review
Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Apologies if there are too many commits, I'm getting on the 'hg wip', 'hg histedit', 'firefoxtrees' train and off of the MQ train.
Comment on attachment 8821305 [details]
Bug 1325464 - Use ES6 method syntax for preferences.

https://reviewboard.mozilla.org/r/100582/#review101428
Attachment #8821305 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821306 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/100584/#review101430

::: browser/components/preferences/in-content/advanced.js:21
(Diff revision 1)
> -  init: function()
> +  init()
>    {

If you want to do another bug to move the braces to the same line I won't complain :)
Attachment #8821306 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821307 [details]
Bug 1325464 - Use ES6 method syntax for applications.js.

https://reviewboard.mozilla.org/r/100586/#review101432
Attachment #8821307 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821308 [details]
Bug 1325464 - Use ES6 method syntax for content.js.

https://reviewboard.mozilla.org/r/100588/#review101434
Attachment #8821308 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821309 [details]
Bug 1325464 - Use ES6 method syntax for main.js.

https://reviewboard.mozilla.org/r/100590/#review101436
Attachment #8821309 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821310 [details]
Bug 1325464 - Use ES6 method syntax for security.js.

https://reviewboard.mozilla.org/r/100592/#review101438
Attachment #8821310 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821311 [details]
Bug 1325464 - Use ES6 method syntax for subdialogs.js.

https://reviewboard.mozilla.org/r/100594/#review101440
Attachment #8821311 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821312 [details]
Bug 1325464 - Use ES6 method syntax for sync.js.

https://reviewboard.mozilla.org/r/100596/#review101442
Attachment #8821312 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821313 [details]
Bug 1325464 - Use ES6 method syntax for search.js.

https://reviewboard.mozilla.org/r/100598/#review101444
Attachment #8821313 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821314 [details]
Bug 1325464 - Use ES6 method syntax for privacy.js.

https://reviewboard.mozilla.org/r/100600/#review101446

::: browser/components/preferences/in-content/privacy.js:29
(Diff revision 1)
>  
>    /**
>     * Show the Tracking Protection UI depending on the
>     * privacy.trackingprotection.ui.enabled pref, and linkify its Learn More link
>     */
> -  _initTrackingProtection: function() {
> +  _initTrackingProtection() {

Please enforce this with eslint: http://eslint.org/docs/rules/object-shorthand

It even supports `--fix` so you can make sure you didn't miss anything.
Attachment #8821314 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
The last two patches I have uploaded are in response to comment #21. I have expanded the scope of this bug to fix all of /toolkit and /browser. https://reviewboard.mozilla.org/r/101196/diff/#index_header was auto-generated using --fix.
Summary: Use ES6 method definition syntax in /browser/components/preferences → Enable object-shortand eslint rules for toolkit
(Assignee)

Updated

6 months ago
Summary: Enable object-shortand eslint rules for toolkit → Enable object-shorthand eslint rules for toolkit
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

6 months ago
mozreview-review-reply
Comment on attachment 8821306 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/100584/#review101430

> If you want to do another bug to move the braces to the same line I won't complain :)

Yeah, I'll fix that in another bug. Hopefully with an eslint rule to help keep us consistent.
Comment on attachment 8822216 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/101196/#review101896

This looks fine to me but I wonder if there should be a chance for people to voice why we should potentially wait on this as it may break some older tools (e.g. editor functionality to jump to a method definition?).

::: browser/base/content/aboutDialog-appUpdater.js:164
(Diff revision 2)
>        return Services.prefs.getBoolPref("app.update.auto");
>      }
>      catch (e) { }
>      return true; // Firefox default is true
>    },
>  

Your commit message seems to have two messages folded together which should be fixed. If you're using histedit you could have used `roll` instead of `fold` to avoid that.

::: toolkit/components/captivedetect/captivedetect.js:338
(Diff revision 2)
>      };
>  
>      this._runningRequest['urlFetcher'] = urlFetcher;
>    },
>  
>    _startLogin: function _startLogin() {

`--fix` understandably doesn't seem to touch cases where the function name matches the property name like this so it's possible that this patch is introducing inconsitency of shorthand vs. longhand with a function name in the same object but in those cases there was already inconsitency of longhand without vs. with a function name so I think that's okay.
Attachment #8822216 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8822217 [details]
Bug 1325464 - Manually fix the remaining ES6 method syntax eslint errors.

https://reviewboard.mozilla.org/r/101198/#review101900
Attachment #8822217 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

6 months ago
mozreview-review-reply
Comment on attachment 8822216 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/101196/#review101896

SublimeText3 supports it and I see that VisualStudio also supports it. I'm not sure what other tools people may be using, but we've had usages of it in our code since they were introduced around Firefox33/34 so I'm OK with pushing this forward.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8821307 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821308 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821309 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821310 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821311 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821312 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821313 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8821314 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8822216 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 61

6 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4079437c4648
Use ES6 method syntax for preferences. r=MattN
https://hg.mozilla.org/integration/autoland/rev/cd10db6087dd
Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled. r=MattN
https://hg.mozilla.org/integration/autoland/rev/562ddc32cc21
Manually fix the remaining ES6 method syntax eslint errors. r=MattN
(Assignee)

Updated

6 months ago
Component: Preferences → General
Product: Firefox → Toolkit
I had to back these out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=65324637&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/6fb777f48fa6
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 65

6 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01d6a34ee366
Use ES6 method syntax for preferences. r=MattN
https://hg.mozilla.org/integration/autoland/rev/15ea2e33fcd4
Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e551e215986a
Manually fix the remaining ES6 method syntax eslint errors. r=MattN
(Assignee)

Updated

6 months ago
Flags: needinfo?(jaws)

Comment 66

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01d6a34ee366
https://hg.mozilla.org/mozilla-central/rev/15ea2e33fcd4
https://hg.mozilla.org/mozilla-central/rev/e551e215986a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1326305
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> Comment on attachment 8821306 [details]
> Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with
> the rule enabled.
> 
> https://reviewboard.mozilla.org/r/100584/#review101430
> 
> > If you want to do another bug to move the braces to the same line I won't complain :)
> 
> Yeah, I'll fix that in another bug. Hopefully with an eslint rule to help
> keep us consistent.

Filed bug 1326511 for this work.
You need to log in before you can comment on or make changes to this bug.