Issue 427 - Remove non-standard function and getter syntax

Remove non-standard function and getter syntax

[tschuster, 2014-05-05 15:30:29]

review please

[Wladimir Palant, 2014-05-05 15:45:40]

IMHO we should use fat arrow functions instead of expression closures - for the
sake of readability. Won't work for getters but better than nothing. The
conversion in JSHydra should be adjusted accordingly of course.

[tschuster, 2014-05-05 17:36:43]

On 2014/05/05 15:45:40, Wladimir Palant wrote:
> IMHO we should use fat arrow functions instead of expression closures - for
the
> sake of readability. Won't work for getters but better than nothing. The
> conversion in JSHydra should be adjusted accordingly of course.

Do you want to desugar fat arrow functions to bound functions with JSHydra?
Those might have somewhat worse performance.

[Wladimir Palant, 2014-05-05 17:59:08]

On 2014/05/05 17:36:43, tschuster wrote:
> Do you want to desugar fat arrow functions to bound functions with JSHydra?
> Those might have somewhat worse performance.

Yes, that's the preferable solution. Ideally we would check whether the "this"
keyword is used in the function body and only bind them in that case.

[tschuster, 2014-05-10 12:43:23]

Convert shorthand function expressions to arrow functions, this of course isn't
possible for getter/setters

[Wladimir Palant, 2014-05-15 07:12:38]

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/appSupport.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/appSupport.js:31: exports.isKnownWindow = function isKnownWindow(/**Window*/
window) {
(/**Window*/ window) => false?

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/appSupport.js:271: };
(window) => window.gBrowser?

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/appSupport.js:405: }
(window) => window.getBrowser()?

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/appSupport.js:702: };
Use fat arrow expressions here?

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/elemHide.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/elemHide.js:382: get styleURL() {
Nit: Bracket should go on the next line.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/filterClasses.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/filterClasses.js:242: get disabled() {
Nit: Bracket on the next line please, same for the getters below.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/filterStorage.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/filterStorage.js:50: get formatVersion() { return formatVersion },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/requestNotifier.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/requestNotifier.js:351: get typeDescr() { return
require("contentPolicy").Policy.typeDescr[this.type]; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/requestNotifier.js:356: get localizedDescr() { return
require("contentPolicy").Policy.localizedDescr[this.type]; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/subscriptionClasses.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:71: get title() { return this._title; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:87: get fixedTitle() { return this._fixedTitle; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:103: get disabled() { return this._disabled; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:369: get homepage() { return this._homepage; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:385: get lastDownload() { return this._lastDownload;
},
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:461: get downloadStatus() { return
this._downloadStatus; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:482: get lastCheck() { return this._lastCheck; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/subscriptionClasses.js:510: get errors() { return this._errors; },
Nit: Please add proper line breaks here.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
File lib/ui.js (right):

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/ui.js:130: && subscription.url == Prefs.subscriptions_exceptionsurl);
It should be either operator on new line (like here) or operator before the line
break (like in your next change), not both. I think the latter is what we
usually do.

http://codereview.adblockplus.org/6305806509146112/diff/5724160613416960/lib/...
lib/ui.js:768: if (FilterStorage.subscriptions.some(subscription =>
subscription.url == Prefs.subscriptions_exceptionsurl || subscription.url in
privacySubscriptions))
According to
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/arrow_functions
the syntax without parentheses around a single parameter is apparently legit.
However, I wonder whether we should still always use parentheses for
consistency. Note that you always had parentheses in your changes before this
point.

[tschuster, 2014-05-18 10:51:34]

Wow sorry for those wrong braces, I am so used to a different style that I
didn't even realize what I was doing.

[tschuster, 2014-05-24 11:10:32]

r?

[Wladimir Palant, 2014-05-25 08:27:55]

LGTM