Issue 2257 - Replaced non-standard function expressions with ES6 arrow functions

Issue 2257 - Replaced non-standard function expressions with ES6 arrow functions

[tschuster, 2015-04-13 10:12:04]

Rest looks good to me.

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
File chrome/content/ui/composer.js (right):

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
chrome/content/ui/composer.js:165: typeNode.addEventListener("command", () =>
checkboxUpdated(this), false);
I think |this| here would be wrong. It's probably the global and not the
typeNode.

[tschuster, 2015-04-13 10:13:14]

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
File chrome/content/ui/sidebar.js (right):

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
chrome/content/ui/sidebar.js:555: let domains = match[1].split("|").filter(d =>
d != domain && d != "~" + domain && (d.length <= domain.length ||
d.lastIndexOf("." + domain) != d.length - domain.length - 1));
And wow that thing! :)

[Sebastian Noack, 2015-04-13 10:22:03]

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
File chrome/content/ui/composer.js (right):

http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
chrome/content/ui/composer.js:165: typeNode.addEventListener("command", () =>
checkboxUpdated(this), false);
On 2015/04/13 10:12:04, tschuster wrote:
> I think |this| here would be wrong. It's probably the global and not the
> typeNode.

Well spotted.

[tschuster, 2015-04-13 11:29:22]

On 2015/04/13 10:22:03, Sebastian Noack wrote:
>
http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
> File chrome/content/ui/composer.js (right):
> 
>
http://codereview.adblockplus.org/4884233277407232/diff/5724160613416960/chro...
> chrome/content/ui/composer.js:165: typeNode.addEventListener("command", () =>
> checkboxUpdated(this), false);
> On 2015/04/13 10:12:04, tschuster wrote:
> > I think |this| here would be wrong. It's probably the global and not the
> > typeNode.
> 
> Well spotted.

LGTM

[Wladimir Palant, 2015-04-13 22:05:05]

While this change does look good, please make sure that each code path you
changed here is tested (no, unit tests don't cover UI code). If you cannot do
that - well, maybe that refactoring isn't really necessary after all. I'm
currently undecided as to whether this issue should be "ready" or "rejected".

[tschuster, 2015-04-14 09:51:00]

On 2015/04/13 22:05:05, Wladimir Palant wrote:
> While this change does look good, please make sure that each code path you
> changed here is tested (no, unit tests don't cover UI code). If you cannot do
> that - well, maybe that refactoring isn't really necessary after all. I'm
> currently undecided as to whether this issue should be "ready" or "rejected".

Unless something inside the arrow function is using |this|, I can't imagine any
scenario where replacing function expressions would somehow change behavior.

[Wladimir Palant, 2015-04-14 12:49:29]

On 2015/04/14 09:51:00, tschuster wrote:
> Unless something inside the arrow function is using |this|, I can't imagine
any
> scenario where replacing function expressions would somehow change behavior.

Sure. But guess how often it happens that a seemingly trivial change breaks
something ;)

[tschuster, 2015-05-06 10:12:32]

On 2015/04/14 12:49:29, Wladimir Palant wrote:
> On 2015/04/14 09:51:00, tschuster wrote:
> > Unless something inside the arrow function is using |this|, I can't imagine
> any
> > scenario where replacing function expressions would somehow change behavior.
> 
> Sure. But guess how often it happens that a seemingly trivial change breaks
> something ;)

I will ping this bug again before Firefox starts showing a warning for it:
https://bugzilla.mozilla.org/show_bug.cgi?id=995610

[Sebastian Noack, 2015-05-06 10:54:26]

On 2015/05/06 10:12:32, tschuster wrote:
> On 2015/04/14 12:49:29, Wladimir Palant wrote:
> > On 2015/04/14 09:51:00, tschuster wrote:
> > > Unless something inside the arrow function is using |this|, I can't
imagine
> > any
> > > scenario where replacing function expressions would somehow change
behavior.
> > 
> > Sure. But guess how often it happens that a seemingly trivial change breaks
> > something ;)
> 
> I will ping this bug again before Firefox starts showing a warning for it:
> https://bugzilla.mozilla.org/show_bug.cgi?id=995610

Thanks for the reminder. I just rebased the patch. But the problem is still that
most code paths this patch touches haven't been tested yet. And testing all of
them is quite some work. Feel free to help testing those.