Issue 3869 - Use the new FilterNotifier API

Issue 3869 - Use the new FilterNotifier API

[Sebastian Noack, 2016-03-23 23:15:51]

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:287: else
I wonder whether we should prehaps unsupport calling the *.listen messages with
null? Since this logic is duplicated 4 times, it would save us quite some
boilerplate. And the UI can still achieve the same by sending and empty array.

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:388:
FilterNotifier.triggerListeners("subscription.disabled",
This belongs into the mock implementation. Otherwise, subscription.disabled
notifications are dispatched redundantly in the extension. While I had to adapt
that line anyway, I removed it and fixed the mock implementation instead.

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:65: function sendMessage(type, action)
Sorry for refactoring sendMessage() again, but with onFilterChange gone, it made
sense to move the filter/subscription conversion in here. Also "args" are now
accepted as variable arguments instead of an array.

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:99: function addFilterListeners(type, actions)
Same pattern as for Prefs.on(), the first UI that listens for a particular
preference causes a listener for that preference to be added, which then
propagates the notifications to all pages that listen for it.

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:115: for (var i = 0; i < arguments.length; i++)
For the record, simply passing the arguments object to sendMessage() or whatever
function would cause a deoptimization in V8. So we need a loop to turn it into
an array anyway.

[Thomas Greiner, 2016-03-24 14:12:17]

LGTM

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:388:
FilterNotifier.triggerListeners("subscription.disabled",
On 2016/03/23 23:15:51, Sebastian Noack wrote:
> This belongs into the mock implementation. Otherwise, subscription.disabled
> notifications are dispatched redundantly in the extension. While I had to
adapt
> that line anyway, I removed it and fixed the mock implementation instead.

I agree. Well spotted.

[Thomas Greiner, 2016-03-24 14:22:35]

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:287: else
On 2016/03/23 23:15:51, Sebastian Noack wrote:
> I wonder whether we should prehaps unsupport calling the *.listen messages
with
> null? Since this logic is duplicated 4 times, it would save us quite some
> boilerplate. And the UI can still achieve the same by sending and empty array.

Sorry, missed this comment.

Yeah, in one of the previous reviews I noted that overloading "*.listen" is not
a great solution in general and that an explicit "*.unlisten" message might be
more appropriate. So by splitting that off, "filter" would no longer be optional
and therefore wouldn't be allowed to be `null` anymore.

[Sebastian Noack, 2016-03-24 14:40:35]

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29339034/diff/29339035/messageResponder.js...
messageResponder.js:287: else
On 2016/03/24 14:22:35, Thomas Greiner wrote:
> On 2016/03/23 23:15:51, Sebastian Noack wrote:
> > I wonder whether we should prehaps unsupport calling the *.listen messages
> with
> > null? Since this logic is duplicated 4 times, it would save us quite some
> > boilerplate. And the UI can still achieve the same by sending and empty
array.
> 
> Sorry, missed this comment.
> 
> Yeah, in one of the previous reviews I noted that overloading "*.listen" is
not
> a great solution in general and that an explicit "*.unlisten" message might be
> more appropriate. So by splitting that off, "filter" would no longer be
optional
> and therefore wouldn't be allowed to be `null` anymore.

So I read that as you are fine with me simplifying the code by unsupporting
null. There you go, new patch set is up.

As for adding *.unlisten, I don't think that this is necessary. As discussed on
the previous review, we cannot send messages on unload to unregister listeners
anyway, neither do we have to with the current approach. And if in fact, a UI is
just temporarily interested in particular events, it can still update the events
to be listened for by calling *.listen again with an empty array.

[Thomas Greiner, 2016-03-24 15:15:05]

LGTM