Issue 4580 - Replace ext.backgroundPage.sendMessage with runtime.sendMessage

Issue 4580 - Replace ext.backgroundPage.sendMessage with runtime.sendMessage

Related: https://codereview.adblockplus.org/29573726/

[Manish Jethani, 2017-10-11 15:39:06]

Patch Set 1

[Manish Jethani, 2017-10-11 15:42:41]

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js
File ext/content.js (left):

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js#oldc...
ext/content.js:32: }, false);
Not related but might as well.

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js#newc...
ext/content.js:62: /* Polyfills */
This will move to polyfill.js as part of the other patch.

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js#newc...
ext/content.js:79: let callbackWrapper = event =>
Just modernized the code a bit, using arrow functions and dropping the false
argument to addEventListener.

[Thomas Greiner, 2017-10-11 15:58:16]

LGTM

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js#newc...
ext/content.js:64: if (!("runtime" in chrome))
Suggestion: `chrome` is defined in `ext/common.js` so I'd recommend indicating
that via `/* globals chrome */` below the license header text

[Manish Jethani, 2017-10-11 16:11:20]

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29573735/diff/29573736/ext/content.js#newc...
ext/content.js:64: if (!("runtime" in chrome))
On 2017/10/11 15:58:16, Thomas Greiner wrote:
> Suggestion: `chrome` is defined in `ext/common.js` so I'd recommend indicating
> that via `/* globals chrome */` below the license header text

Is that really necessary? The chrome object is used all over the place, ESLint
never complains about this, I think because we've turned on that option in the
config.

If we do this though then we should do it in other files as well, in that case
as a separate commit.

[Thomas Greiner, 2017-10-11 16:46:40]

On 2017/10/11 16:11:20, Manish Jethani wrote:
> Is that really necessary? The chrome object is used all over the place, ESLint
> never complains about this, I think because we've turned on that option in the
> config.

No, not necessary. It was merely a suggestion because I noticed it being used in
adblockplusui/common.js.

[Manish Jethani, 2017-10-11 16:57:58]

On 2017/10/11 16:46:40, Thomas Greiner wrote:
> On 2017/10/11 16:11:20, Manish Jethani wrote:
> > Is that really necessary? The chrome object is used all over the place,
ESLint
> > never complains about this, I think because we've turned on that option in
the
> > config.
> 
> No, not necessary. It was merely a suggestion because I noticed it being used
in
> adblockplusui/common.js.

Acknowledged.

Thanks!

[Sebastian Noack, 2017-10-13 03:07:21]

On 2017/10/11 16:57:58, Manish Jethani wrote:
> On 2017/10/11 16:46:40, Thomas Greiner wrote:
> > On 2017/10/11 16:11:20, Manish Jethani wrote:
> > > Is that really necessary? The chrome object is used all over the place,
> ESLint
> > > never complains about this, I think because we've turned on that option in
> the
> > > config.
> > 
> > No, not necessary. It was merely a suggestion because I noticed it being
used
> in
> > adblockplusui/common.js.
> 
> Acknowledged.
> 
> Thanks!

env.webextensions is set totrue in eslintrc.json, in order to let ESLint know
that the code is expected to run in a web extension context, where the "browser"
and/or "chrome" objects are globally available:
https://hg.adblockplus.org/adblockplusui/file/71690e7b23cc/.eslintrc.json#l6

Using /* global chrome */ is redundant and doesn't make any sense to me.