Issue 4783 - Use Port API for the messageResponder

Issue 4783 - Use Port API for the messageResponder

[kzar, 2017-01-11 08:48:25]

Patch Set 1

This change is required for my continuing work to target the browserext API in
adblockpluschrome, but I also think it's generally a good idea. Having all
message handlers in one massive function seems to be getting pretty unwieldy.
(Perhaps in the future we'd like to move the listeners closer to their related
code too, similar to recent changes in adblockpluschrome which are working
towards removing background.js. But I'm getting offtopic now...)

I've tested this as best I can, by making sure nothing in the options page or in
general use breaks. I could have missed something however so this will likely
need quite extensive QA.

[Sebastian Noack, 2017-01-11 16:48:20]

Did you do any changes other than indentation to any code inside the handlers?
If so can you point them out.

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:161: port.on("app.get", (message, sender) =>
I wonder whether we should ignore the sender argument in handlers that don't use
it, just expecting the message argument:

  port.on("app.get", message =>
  {
    ...
  });

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:168: legacySafariVersion: (info.platform == "safari" && (
Since we are in the process of removing Safari support, this logic and the
corresponding code on the content side is obsolete now. Removing it is arguably
out of scope for this change though, but while on it, perhaps you want to remove
it with another patch?

[kzar, 2017-01-12 10:07:50]

> Did you do any changes other than indentation to any code 
> inside the handlers?

IIRC not really. I did have to change callbacks to use returns and I had to be
careful in message handlers which mixed both returns and callbacks.
Unfortunately the diff provided isn't too helpful sorry, if it helps here's the
commit on GitHub
https://github.com/kzar/adblockplusui/commit/8d43a35d909da1b239ecdf822d0252dc...

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:161: port.on("app.get", (message, sender) =>
On 2017/01/11 16:48:20, Sebastian Noack wrote:
> I wonder whether we should ignore the sender argument in handlers that don't
use
> it, just expecting the message argument:
> 
>   port.on("app.get", message =>
>   {
>     ...
>   });

Personally I think it's better to list all the parameters in the function
definition even if they're not used.

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:168: legacySafariVersion: (info.platform == "safari" && (
On 2017/01/11 16:48:20, Sebastian Noack wrote:
> Since we are in the process of removing Safari support, this logic and the
> corresponding code on the content side is obsolete now. Removing it is
arguably
> out of scope for this change though, but while on it, perhaps you want to
remove
> it with another patch?

Go on then :p https://codereview.adblockplus.org/29371585/

[Sebastian Noack, 2017-01-12 10:29:02]

LGTM

[Sebastian Noack, 2017-01-12 11:17:59]

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:23: var port = require("messaging").port;
I might have judged a little too prematurely. It seems the "messaging" module
isn't implemented in the mock backend (i.e. background.js). So unless I miss
something, if you test the UI standalone (outside of the extension), this will
fail.

[kzar, 2017-01-13 08:02:41]

Patch Set 2 : Add polyfilly for messaging API to background.js

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29370996/diff/29370997/messageResponder.js...
messageResponder.js:23: var port = require("messaging").port;
On 2017/01/12 11:17:59, Sebastian Noack wrote:
> I might have judged a little too prematurely. It seems the "messaging" module
> isn't implemented in the mock backend (i.e. background.js). So unless I miss
> something, if you test the UI standalone (outside of the extension), this will
> fail.

Argh, you're right. I've had a go at implementing a polyfill for that.

[Sebastian Noack, 2017-01-13 12:12:10]

https://codereview.adblockplus.org/29370996/diff/29371679/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newco...
background.js:341: modules.messaging = {
Can you please just use the EventEmitter class here, rather than duplicating the
very same logic?

https://codereview.adblockplus.org/29370996/diff/29371679/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29370996/diff/29371679/messageResponder.js...
messageResponder.js:160: global.ext.onMessage.addListener(function(message,
sender, callback)
I suppose the mock implementation for ext.onMessage can be removed now?

[kzar, 2017-01-16 04:27:02]

https://codereview.adblockplus.org/29370996/diff/29371679/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newco...
background.js:341: modules.messaging = {
On 2017/01/13 12:12:10, Sebastian Noack wrote:
> Can you please just use the EventEmitter class here, rather than duplicating
the
> very same logic?

Well I'm not sure I can since the EventEmitter class is defined in
adblockpluschrome not this repository. Unless you mean for me to copy and paste
the whole thing in here?

https://codereview.adblockplus.org/29370996/diff/29371679/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29370996/diff/29371679/messageResponder.js...
messageResponder.js:160: global.ext.onMessage.addListener(function(message,
sender, callback)
On 2017/01/13 12:12:10, Sebastian Noack wrote:
> I suppose the mock implementation for ext.onMessage can be removed now?

Not yet unfortunately, it's still used in a few other places :/

[Sebastian Noack, 2017-01-16 13:46:38]

https://codereview.adblockplus.org/29370996/diff/29371679/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newco...
background.js:341: modules.messaging = {
On 2017/01/16 04:27:02, kzar wrote:
> On 2017/01/13 12:12:10, Sebastian Noack wrote:
> > Can you please just use the EventEmitter class here, rather than duplicating
> the
> > very same logic?
> 
> Well I'm not sure I can since the EventEmitter class is defined in
> adblockpluschrome not this repository. Unless you mean for me to copy and
paste
> the whole thing in here?

See line 20 in this file. There is a class called EventEmitter which you could
use here.

[kzar, 2017-01-16 14:25:56]

Patch Set 3 : Use EventEmitter

https://codereview.adblockplus.org/29370996/diff/29371679/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29371679/background.js#newco...
background.js:341: modules.messaging = {
On 2017/01/16 13:46:38, Sebastian Noack wrote:
> On 2017/01/16 04:27:02, kzar wrote:
> > On 2017/01/13 12:12:10, Sebastian Noack wrote:
> > > Can you please just use the EventEmitter class here, rather than
duplicating
> > the
> > > very same logic?
> > 
> > Well I'm not sure I can since the EventEmitter class is defined in
> > adblockpluschrome not this repository. Unless you mean for me to copy and
> paste
> > the whole thing in here?
> 
> See line 20 in this file. There is a class called EventEmitter which you could
> use here.

Oh yea, sorry I missed that. Done.

[Sebastian Noack, 2017-01-16 15:00:03]

https://codereview.adblockplus.org/29370996/diff/29372046/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29372046/background.js#newco...
background.js:342: _eventEmitter: new EventEmitter(),
Couldn't you do it like that:

  modules.messaging = {
    port: new EventEmitter()
  };

  window.addEventListener("message", event =>
  {
    // code from patch set #2 here but simply using
    // modules.messaging.port._listeners instead of messageListeners
  });

This way we wouldn't need to wrap the EventEmitter, neither do we export
private/prefixed APIs.

[kzar, 2017-01-16 15:16:31]

Patch Set 4 : Improve messaging implementation in background.js

https://codereview.adblockplus.org/29370996/diff/29372046/background.js
File background.js (right):

https://codereview.adblockplus.org/29370996/diff/29372046/background.js#newco...
background.js:342: _eventEmitter: new EventEmitter(),
On 2017/01/16 15:00:03, Sebastian Noack wrote:
> Couldn't you do it like that:
> 
>   modules.messaging = {
>     port: new EventEmitter()
>   };
> 
>   window.addEventListener("message", event =>
>   {
>     // code from patch set #2 here but simply using
>     // modules.messaging.port._listeners instead of messageListeners
>   });
> 
> This way we wouldn't need to wrap the EventEmitter, neither do we export
> private/prefixed APIs.

Done.

[Sebastian Noack, 2017-01-16 16:01:25]

LGTM

[Thomas Greiner, 2017-01-17 17:14:42]

LGTM