Issue 3823 - Split up message responder code

Issue 3823 - Split up message responder code

[kzar, 2016-03-18 15:04:07]

A lot of these changes seem to be unrelated renaming of event messages, are they
down to rebasing? (If not I think they should be split out from this review.)

[Sebastian Noack, 2016-03-18 15:08:57]

Besides the renamed messages I'm not sure which supposedly unrelated changes you
mean?

As for the renamed messages, while changing that code anyway, it makes sense to
adapt to the new naming scheme that is already used consistently by adlokplusui.

[kzar, 2016-03-18 15:22:11]

I think the new port API is really cool, looks much friendlier to use. I think
Wladimir should probably also check its implementation in lib/messaging.js
though.

On 2016/03/18 15:08:57, Sebastian Noack wrote:
> As for the renamed messages, while changing that code anyway, it makes sense
to
> adapt to the new naming scheme that is already used consistently by
adlokplusui.

Well for example changing the names of the messages inside
blockElement.postload.js seemed kind of unrelated, but looking at the changes to
lib/filterComposer.js I see what you mean. Fair enough.

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer....
lib/filterComposer.js:69: /**
Any reason you deleted this documentation? (I understand it's no longer an
exported function but what's the harm in leaving it in? It could still be useful
to someone who is trying to understand the code.)

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js#ne...
lib/messaging.js:30: ext.onMessage.addListener(this._onMessage.bind(this));
I guess this could be a memory leak, if the port is deleted the onMessage
listener remains?

[Sebastian Noack, 2016-03-18 15:26:22]

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer....
lib/filterComposer.js:69: /**
On 2016/03/18 15:22:10, kzar wrote:
> Any reason you deleted this documentation? (I understand it's no longer an
> exported function but what's the harm in leaving it in? It could still be
useful
> to someone who is trying to understand the code.)

I found the documentation for this function quite long given that it's mostly
boilerplate not really adding anything that isn't already obvious. So I used the
chance to remove that documentation while it's no longer a public function.

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js#ne...
lib/messaging.js:30: ext.onMessage.addListener(this._onMessage.bind(this));
On 2016/03/18 15:22:10, kzar wrote:
> I guess this could be a memory leak, if the port is deleted the onMessage
> listener remains?

Who cares? This class is used as a singleton.

[kzar, 2016-03-18 15:52:40]

Otherwise LGTM

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/filterComposer....
lib/filterComposer.js:69: /**
On 2016/03/18 15:26:21, Sebastian Noack wrote:
> On 2016/03/18 15:22:10, kzar wrote:
> > Any reason you deleted this documentation? (I understand it's no longer an
> > exported function but what's the harm in leaving it in? It could still be
> useful
> > to someone who is trying to understand the code.)
> 
> I found the documentation for this function quite long given that it's mostly
> boilerplate not really adding anything that isn't already obvious. So I used
the
> chance to remove that documentation while it's no longer a public function.

I think it's useful but if you disagree I guess we can leave the deciding vote
to Wlaidmir.

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338492/lib/messaging.js#ne...
lib/messaging.js:30: ext.onMessage.addListener(this._onMessage.bind(this));
On 2016/03/18 15:26:22, Sebastian Noack wrote:
> On 2016/03/18 15:22:10, kzar wrote:
> > I guess this could be a memory leak, if the port is deleted the onMessage
> > listener remains?
> 
> Who cares? This class is used as a singleton.

Acknowledged.

[Sebastian Noack, 2016-03-19 19:45:22]

Rebased and moved changes related to renaming composer (aka "Block element")
message types to https://codereview.adblockplus.org/29338503/ to make this patch
easier to review.

[kzar, 2016-03-20 14:39:08]

LGTM

(Sorry I didn't mean to slow things down by asking if Wladimir could double
check lib/messaging.js as well. I don't mind if you want to go ahead and push,
especially as this is now blocking other changes.)

[Wladimir Palant, 2016-03-21 11:59:41]

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:43: if (response instanceof Promise)
This is a bad idea. There can be multiple declarations of the Promise class
(e.g. the native one and a shim, also the same class but in the context of a
different frame), and there can be other classes that should be treated like
promises - e.g. Task. It seems that the usual check is `response && typeof
response.then == "function"` (note how
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
is talking about "thenable" values). Erik suggested encapsulating this check in
an isPromise() function in the other review.

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:49: sendResponse(undefined);
I guess that you plan adding Port.emit() as well so that undefined is handled
properly? Currently the behavior is inconsistent: undefined will be suppressed
for synchronous responses but passed as a response to the caller for
asynchronous ones.

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:84: this._callbacks[name].push(callback);
The Firefox implementation also ensures that the same callback isn't added
twice. Without this check a callback could still be called even after calling
port.off() - because the second instance stays there. And of course it will be
called twice. IMHO this is inconsistent und unexpected behavior.

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js#newcode83
popup.js:83: port.off("composer.ready", onComposerReady);
IMHO, this shouldn't be necessary - each context (here: window) should have its
own port that should detach automatically when that context becomes invalid. In
Chrome this probably means no default port but rather:

  var port = require("messaging").getPort(window);

[Sebastian Noack, 2016-03-21 13:26:03]

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:43: if (response instanceof Promise)
On 2016/03/21 11:59:40, Wladimir Palant wrote:
> This is a bad idea. There can be multiple declarations of the Promise class
> (e.g. the native one and a shim, also the same class but in the context of a
> different frame), and there can be other classes that should be treated like
> promises - e.g. Task. It seems that the usual check is `response && typeof
> response.then == "function"` (note how
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> is talking about "thenable" values). Erik suggested encapsulating this check
in
> an isPromise() function in the other review.

I don't think that this is a concern here, since this code is only used in the
background page, so Promise is always either the builtin, or always our
polyfill. But on the other hand the response cannot have any complex values
(like functions) anyway. So fine with me.

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:49: sendResponse(undefined);
On 2016/03/21 11:59:40, Wladimir Palant wrote:
> I guess that you plan adding Port.emit() as well so that undefined is handled
> properly? Currently the behavior is inconsistent: undefined will be suppressed
> for synchronous responses but passed as a response to the caller for
> asynchronous ones.

Well, if the promise is rejected we have a bug anyway. So IMO it doesn't matter
what we do then. But yes, if we are going to implement Port.emit() and use it on
the other end we could properly handle that case. But even then I'm not sure
whether it will be worth it.

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:84: this._callbacks[name].push(callback);
On 2016/03/21 11:59:40, Wladimir Palant wrote:
> The Firefox implementation also ensures that the same callback isn't added
> twice. Without this check a callback could still be called even after calling
> port.off() - because the second instance stays there. And of course it will be
> called twice. IMHO this is inconsistent und unexpected behavior.

Actually, the implementation and behavior here is consistent with node.js. And
FWIW I think it's a rather theoretical scenario with no real use case that
doesn't justify any addional complexity to be handled here.

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js#newcode83
popup.js:83: port.off("composer.ready", onComposerReady);
On 2016/03/21 11:59:40, Wladimir Palant wrote:
> IMHO, this shouldn't be necessary - each context (here: window) should have
its
> own port that should detach automatically when that context becomes invalid.
In
> Chrome this probably means no default port but rather:
> 
>   var port = require("messaging").getPort(window);

If we would use window.chrome.runtime.onMessage under the hood that shouldn't be
necessary. However, since we currently have no ways to load modules in the
content process, require() and the modules it returns are always the objects
from the background page. Hence extension APIs called under the hood are invoked
in the context of the background page.

Moreover, we currently import the ext namespace from the background page, rather
than including those scripts in the browser action popup. I only vaguely
remember the reason for that, but this makes using the chrome API in the context
of the popup (rather than the background page) even more difficult.

Yes, as a workaround we could implement something like getPort(window) which
essentially would have to register an unload event to detach message listeners.
But that would just move this hack from one place to another introducing even
more complexity. Also note that the code here, including this hack, might likely
get refactored in order to use asynchronous messaging, at some point, anyway.

[Wladimir Palant, 2016-03-21 13:58:01]

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/lib/messaging.js#ne...
lib/messaging.js:84: this._callbacks[name].push(callback);
On 2016/03/21 13:26:02, Sebastian Noack wrote:
> Actually, the implementation and behavior here is consistent with node.js.

You are right. I verified that Add-on SDK eliminates duplicate listeners whereas
Node.js does not. Let's stick with Node.js behavior then, I'll adjust the
Firefox implementation accordingly.

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js#newcode83
popup.js:83: port.off("composer.ready", onComposerReady);
On 2016/03/21 13:26:02, Sebastian Noack wrote:
> Yes, as a workaround we could implement something like getPort(window) which
> essentially would have to register an unload event to detach message
listeners.
> But that would just move this hack from one place to another introducing even
> more complexity.

I disagree. Current API requires developers to remember unregistering event
handlers, if they don't bad things will happen. This just isn't a good way to
design an API.

Sure, you'll need a hack in the API implementation - but it will only be one
hack (a fairly simply one), and you could easily make sure it is implemented
properly. Much better than having hacks everywhere the API is used.

If the code is refactored later and things will work "naturally" - all the
better.

[Sebastian Noack, 2016-03-21 15:26:59]

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338742/popup.js#newcode83
popup.js:83: port.off("composer.ready", onComposerReady);
On 2016/03/21 13:58:01, Wladimir Palant wrote:
> On 2016/03/21 13:26:02, Sebastian Noack wrote:
> > Yes, as a workaround we could implement something like getPort(window) which
> > essentially would have to register an unload event to detach message
> listeners.
> > But that would just move this hack from one place to another introducing
even
> > more complexity.
> 
> I disagree. Current API requires developers to remember unregistering event
> handlers, if they don't bad things will happen. This just isn't a good way to
> design an API.
> 
> Sure, you'll need a hack in the API implementation - but it will only be one
> hack (a fairly simply one), and you could easily make sure it is implemented
> properly. Much better than having hacks everywhere the API is used.
> 
> If the code is refactored later and things will work "naturally" - all the
> better.

Done.

[Wladimir Palant, 2016-03-21 15:50:13]

https://codereview.adblockplus.org/29338491/diff/29338828/background.js
File background.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/background.js#newco...
background.js:43: var port = require("messaging").port;
Arguably, this should use getPort() as well and no default port exposed - if you
expose a default port people will use it by accident and listeners will stay
around.

https://codereview.adblockplus.org/29338491/diff/29338828/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/lib/messaging.js#ne...
lib/messaging.js:130: });
Please consider caching the port per window so that it won't be created multiple
times unnecessarily.

Also, maybe have Port.disconnect() method that's called here like in Firefox?
Note that I export Port constructor. This way the API allows you to create and
disconnect a port manually if necessary.

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js#newcode26
popup.js:26: var getPort = require("messaging").getPort;
I'd really go with:

  var port = require("messaging").getPort(window);

So below you'd just access the port variable. It should only be one port, it's
not an object you create from scratch each time.

[Sebastian Noack, 2016-03-21 17:15:42]

https://codereview.adblockplus.org/29338491/diff/29338828/background.js
File background.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/background.js#newco...
background.js:43: var port = require("messaging").port;
On 2016/03/21 15:50:12, Wladimir Palant wrote:
> Arguably, this should use getPort() as well and no default port exposed - if
you
> expose a default port people will use it by accident and listeners will stay
> around.

background.js is matter to go away. In the mid-term all code will be
encapsulated in modules. This patch is just the first step. And I'd rather avoid
unnecessary boilerplate to get the port in each module, as well as logic to
cache the port which wouldn't be necessary otherwise.

I consider getPort() a temporarily hack, that is matter to be removed once we
use asynchronous messaging properly in the popup. And except for that edge case,
all messaging code using the port API runs in the background page (hence in the
same thread and for the same window) anyway, where we don't have to disconnect
the port.

https://codereview.adblockplus.org/29338491/diff/29338828/lib/messaging.js
File lib/messaging.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/lib/messaging.js#ne...
lib/messaging.js:130: });
On 2016/03/21 15:50:13, Wladimir Palant wrote:
> Also, maybe have Port.disconnect() method that's called here like in Firefox?
> Note that I export Port constructor. This way the API allows you to create and
> disconnect a port manually if necessary.

Done.

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js#newcode26
popup.js:26: var getPort = require("messaging").getPort;
On 2016/03/21 15:50:13, Wladimir Palant wrote:
> I'd really go with:
> 
>   var port = require("messaging").getPort(window);
> 
> So below you'd just access the port variable. It should only be one port, it's
> not an object you create from scratch each time.

Well, the code below only runs once if at all. So there is no way we will create
more than one port here, but on the other hand, this way we only create the port
(and add an addional ext.onMessage listener) if necessary.

[Wladimir Palant, 2016-03-21 18:05:22]

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js#newcode26
popup.js:26: var getPort = require("messaging").getPort;
On 2016/03/21 17:15:42, Sebastian Noack wrote:
> Well, the code below only runs once if at all. So there is no way we will
create
> more than one port here, but on the other hand, this way we only create the
port
> (and add an addional ext.onMessage listener) if necessary.

Why not call require only in that case then?

[Sebastian Noack, 2016-03-21 19:54:13]

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338491/diff/29338828/popup.js#newcode26
popup.js:26: var getPort = require("messaging").getPort;
On 2016/03/21 18:05:22, Wladimir Palant wrote:
> On 2016/03/21 17:15:42, Sebastian Noack wrote:
> > Well, the code below only runs once if at all. So there is no way we will
> create
> > more than one port here, but on the other hand, this way we only create the
> port
> > (and add an addional ext.onMessage listener) if necessary.
> 
> Why not call require only in that case then?

I lend towards having all imports at the beginning of the script. But fair
enough. Done.

[Wladimir Palant, 2016-03-21 20:46:57]

I cannot say that I am happy with the current state, in particular port/getPort
isn't very maintainable as implemented. Sure, it's "temporary" - but temporary
solutions tend to be surprisingly long-lived. In the end, maintaining this code
will be up to you. So no more objections from my end.

I didn't review the logic changes, it's up to Dave to give you his final OK
here.

[kzar, 2016-03-21 21:09:25]

LGTM

[Sebastian Noack, 2016-03-22 08:25:47]

I also move get-domain-enabled-state out of background.js into the whitelisting
module, and adapted the message name.

[kzar, 2016-03-22 13:41:47]

LGTM double plus