Issue 4871 - Have ESLint pass for the messageResponder

Issue 4871 - Have ESLint pass for the messageResponder

Depends on this review which adds the base ESLint configuration: https://codereview.adblockplus.org/29374555/

[kzar, 2017-02-08 08:39:24]

Patch Set 1

Since the changes required to get all of adblockplusui to pass are absolutely
huge I've decided to start off here. (The messageResponder is a good place to
start since the code needed to change in order for the related adblockpluschrome
changes to work anyway.)

(If you're curious to see the further changes which might be required in the
future I've got a branch here
https://github.com/kzar/adblockplusui/tree/4871-eslint with my WIP.)

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:21: if (typeof ext == "undefined")
(This change is required since we'll be using `let` instead of `var` to declare
`ext` in adblockpluschrome when the related ESLint changes land there.)

[kzar, 2017-02-08 09:06:05]

(This no longer blocks #4864 but I still think it's a good place to start so
leaving it open.)

[Thomas Greiner, 2017-02-08 10:11:09]

Just some preliminary comments while we're waiting for #3692.

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:94: for (let i = 0; i < args.length; i++)
Why not use a for-of loop instead to simplify the code?

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:133: sendMessage(...[type, action].concat(args));
Looks like a more complicated way of writing `sendMessage(type, action,
...args)`.

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:169: let rtl =
Utils.chromeRegistry.isLocaleRTL("adblockplus");
Detail: It'd be great if the variable name would reflect that it's a boolean by
calling it `isRtl` or `isRTL`.

[kzar, 2017-02-08 10:27:24]

Patch Set 2 : Addressed feedback

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:94: for (let i = 0; i < args.length; i++)
On 2017/02/08 10:11:09, Thomas Greiner wrote:
> Why not use a for-of loop instead to simplify the code?

Since we need to replace some arguments in place.

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:133: sendMessage(...[type, action].concat(args));
On 2017/02/08 10:11:09, Thomas Greiner wrote:
> Looks like a more complicated way of writing `sendMessage(type, action,
> ...args)`.

Good point, Done.

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:169: let rtl =
Utils.chromeRegistry.isLocaleRTL("adblockplus");
On 2017/02/08 10:11:09, Thomas Greiner wrote:
> Detail: It'd be great if the variable name would reflect that it's a boolean
by
> calling it `isRtl` or `isRTL`.

Done.

[Thomas Greiner, 2017-02-08 10:37:26]

Looks good now so let's wait for the ESLint configuration to be finalized.

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29374774/diff/29374775/messageResponder.js...
messageResponder.js:94: for (let i = 0; i < args.length; i++)
On 2017/02/08 10:27:24, kzar wrote:
> On 2017/02/08 10:11:09, Thomas Greiner wrote:
> > Why not use a for-of loop instead to simplify the code?
> 
> Since we need to replace some arguments in place.

Ah, don't know how I overlooked that. :p You're of course absolutely right.

Usually we'd use `Array.map()` for that instead of a for-loop so not sure why it
wasn't used here. Anyway, no need to make this change if you consider it to be
out of scope for this review.

[kzar, 2017-02-20 09:59:00]

Closing this and opening a new review for fixing ESLint for the whole of
adblockplusui.