Issue 6440 - Use long-lived connections to listen to extension events

This review contains the following changes:
- UI pages now have to register listeners using `browser.runtime.connect()` instead of `browser.runtime.sendMessage()`
- Background page will respond using `postMessage()` on the port instead of using `sendMessage()` on the page
- Only pass on first argument we receive from `FilterNotifier` listener (none of the UI pages use any of the other ones)

Later on we could also receive all other messages via that very same port that's introduced in this review.

[Thomas Greiner, 2018-03-06 18:59:39]

This is pretty much just the bare minimum that's required to make the UI
responsive again. As mentioned at
https://issues.adblockplus.org/ticket/6440#comment:17 there's still plenty of
room for optimization but I thought it'd be better to tackle such optimizations
in #6450.

Technically, we actually only need to use long-lived connections for
"filters.listen" but since this would create duplicated logic and since we
should switch all messages to use long-lived connections, I've already included
the remaining "*.listen" messages in this review.

[a.giammarchi, 2018-03-07 08:14:29]

https://codereview.adblockplus.org/29715759/diff/29715760/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29715759/diff/29715760/messageResponder.js...
messageResponder.js:134: sendMessage(type, action, args[0]);
I understand you want to keep doors opened in the `sendMessage` callback
signature, but reading FilterNotifier source code it's clear to me not even them
cafre much about extra arguments, undocumented param1, param2, and param3, *but*
they clearly emit an `item`.

Wouldn't be cleaner to have that clear knowledge of the received value in here
too?

`FilterNotifier.on(name, item => sendMessage(type, action, item));`

https://codereview.adblockplus.org/29715759/diff/29715760/messageResponder.js...
messageResponder.js:478: let [type,  action] = message.type.split(".", 2);
WOW, first time in my life I've found a use case for the second argument of a
split!

[Thomas Greiner, 2018-03-07 14:05:31]

https://codereview.adblockplus.org/29715759/diff/29715760/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29715759/diff/29715760/messageResponder.js...
messageResponder.js:134: sendMessage(type, action, args[0]);
On 2018/03/07 08:14:29, a.giammarchi wrote:
> I understand you want to keep doors opened in the `sendMessage` callback
> signature, but reading FilterNotifier source code it's clear to me not even
them
> cafre much about extra arguments, undocumented param1, param2, and param3,
*but*
> they clearly emit an `item`.
> 
> Wouldn't be cleaner to have that clear knowledge of the received value in here
> too?
> 
> `FilterNotifier.on(name, item => sendMessage(type, action, item));`

Done.

[saroyanm, 2018-03-07 17:49:45]

Just a couple of comments.

https://codereview.adblockplus.org/29715759/diff/29716579/firstRun.js
File firstRun.js (right):

https://codereview.adblockplus.org/29715759/diff/29716579/firstRun.js#newcode73
firstRun.js:73: let port = browser.runtime.connect({name: "ui"});
We probably need to update the Mock polyfill as well for runtime.connect ->
https://hg.adblockplus.org/adblockplusui/file/tip/ext/content.js#l64

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js...
messageResponder.js:79: let changeListeners = new ext.PageMap();
Detail: Seems like PageMap Mock can be removed already (this is the only place
we are using it) ->
https://hg.adblockplus.org/adblockplusui/file/tip/ext/background.js#l32

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js...
messageResponder.js:140: function getListenerFilters(page)
Seems like function is not being used anymore.

[Thomas Greiner, 2018-03-07 19:19:39]

https://codereview.adblockplus.org/29715759/diff/29716579/firstRun.js
File firstRun.js (right):

https://codereview.adblockplus.org/29715759/diff/29716579/firstRun.js#newcode73
firstRun.js:73: let port = browser.runtime.connect({name: "ui"});
On 2018/03/07 17:49:44, saroyanm wrote:
> We probably need to update the Mock polyfill as well for runtime.connect ->
> https://hg.adblockplus.org/adblockplusui/file/tip/ext/content.js#l64

Done. Since the issue doesn't occur in the test environment I didn't check
whether it works on that too. Anyway, great that you pointed it out.

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js...
messageResponder.js:79: let changeListeners = new ext.PageMap();
On 2018/03/07 17:49:45, saroyanm wrote:
> Detail: Seems like PageMap Mock can be removed already (this is the only place
> we are using it) ->
> https://hg.adblockplus.org/adblockplusui/file/tip/ext/background.js#l32

Done.

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29715759/diff/29716579/messageResponder.js...
messageResponder.js:140: function getListenerFilters(page)
On 2018/03/07 17:49:45, saroyanm wrote:
> Seems like function is not being used anymore.

Done.

[a.giammarchi, 2018-03-12 12:36:44]

not sure what Manvel thinks but this now LGTM

[saroyanm, 2018-03-12 13:08:28]

Looks good to me as well: LGTM

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js#newco...
ext/common.js:78: class Port
I like that we have started to use javascript classes.

Irrelevant to current review: I wonder if we should have a note in the coding
style to prefer such syntactical sugar over using prototypes ?

(Or is this just a mock and we should use whatever works?)

[a.giammarchi, 2018-03-12 13:17:31]

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js#newco...
ext/common.js:78: class Port
On 2018/03/12 13:08:28, saroyanm wrote:
> I like that we have started to use javascript classes.
> 
> Irrelevant to current review: I wonder if we should have a note in the coding
> style to prefer such syntactical sugar over using prototypes ?
> 
> (Or is this just a mock and we should use whatever works?)

FWIW, when needed, I prefer classes too and I think with current targets it'd be
pointless to prefer prototype over classes, specially because classes are more
powerful (i.e. can really extend builtins instead of just pretending and
failing)

[Thomas Greiner, 2018-03-13 18:24:46]

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js#newco...
ext/common.js:78: class Port
On 2018/03/12 13:17:31, a.giammarchi wrote:
> On 2018/03/12 13:08:28, saroyanm wrote:
> > I like that we have started to use javascript classes.
> > 
> > Irrelevant to current review: I wonder if we should have a note in the
coding
> > style to prefer such syntactical sugar over using prototypes ?
> > 
> > (Or is this just a mock and we should use whatever works?)

Yes, this is just a mock. We're still supporting Opera 39 which doesn't support
classes yet, unfortunately.

> FWIW, when needed, I prefer classes too and I think with current targets it'd
be
> pointless to prefer prototype over classes, specially because classes are more
> powerful (i.e. can really extend builtins instead of just pretending and
> failing)

Since there seem to be agreement that we'd like to have the option to use
classes, I'll create a ticket for dropping support for Opera 39 to 42 so that we
can use them in production code. That may also fit nicely into the code
modernization that's going on in #6425.

[a.giammarchi, 2018-03-14 07:51:03]

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29715759/diff/29716652/ext/common.js#newco...
ext/common.js:78: class Port
On 2018/03/13 18:24:45, Thomas Greiner wrote:
> Yes, this is just a mock. We're still supporting Opera 39 which doesn't
support
> classes yet, unfortunately.

AFAIK Opera 36 already supports Classes, but it's good to bump up Opera version
anyway.
https://caniuse.com/#feat=es6-class

P.S. if you had that `caniuse` script you would write `caniuse class` and see
results ;-)
https://webreflection.github.io/eyeo/utilities/caniuse

[Thomas Greiner, 2018-03-14 09:27:31]

On 2018/03/14 07:51:03, a.giammarchi wrote:
> AFAIK Opera 36 already supports Classes, but it's good to bump up Opera
version
> anyway.
> https://caniuse.com/#feat=es6-class

That's odd because according to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#Bro...
it doesn't. caniuse.com even references this page as a source so either I'm
misreading something or one of those is wrong. Initially I also assumed that
Opera would support classes because it's based on Chromium and usually only lags
behind a couple of versions.

[a.giammarchi, 2018-03-14 09:36:30]

On 2018/03/14 09:27:31, Thomas Greiner wrote:
> On 2018/03/14 07:51:03, a.giammarchi wrote:
> > AFAIK Opera 36 already supports Classes, but it's good to bump up Opera
> version
> > anyway.
> > https://caniuse.com/#feat=es6-class
> 
> That's odd because according to
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#Bro...
> it doesn't. http://caniuse.com even references this page as a source so either
I'm
> misreading something or one of those is wrong. Initially I also assumed that
> Opera would support classes because it's based on Chromium and usually only
lags
> behind a couple of versions.

I went ahead. I've downloaded the first release No. 39
https://www.opera.com/download/index.dml/?custom=yes

I've opened it, and I've written `class A {}`

It's OK, Opera 39 supports classes already.

I'll update the MDN page.

[a.giammarchi, 2018-03-14 09:37:02]

On 2018/03/14 09:36:30, a.giammarchi wrote:
> On 2018/03/14 09:27:31, Thomas Greiner wrote:
> > On 2018/03/14 07:51:03, a.giammarchi wrote:
> > > AFAIK Opera 36 already supports Classes, but it's good to bump up Opera
> > version
> > > anyway.
> > > https://caniuse.com/#feat=es6-class
> > 
> > That's odd because according to
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#Bro...
> > it doesn't. http://caniuse.com even references this page as a source so
either
> I'm
> > misreading something or one of those is wrong. Initially I also assumed that
> > Opera would support classes because it's based on Chromium and usually only
> lags
> > behind a couple of versions.
> 
> I went ahead. I've downloaded the first release No. 39
> https://www.opera.com/download/index.dml/?custom=yes
> 
> I've opened it, and I've written `class A {}`
> 
> It's OK, Opera 39 supports classes already.
> 
> I'll update the MDN page.

P.S. it's good to bump Opera version regardless!

[a.giammarchi, 2018-03-14 09:44:16]

FYI: I've filed a PR to MDN =>
https://github.com/mdn/browser-compat-data/pull/1366

It's also not the first time we base assumptions over MDN but unfortunately MDN
is like any other documentation: it's not always updated and sometimes not fully
reliable. We should always check for real when it's possible (same goes for
caniuse)