Issue 1663 - Emulate background page and implement proper message responder

I realized that the message responder will be the same for all platforms, so I`ve added this one. In order for this to work the background page had to be emulated properly however

[Wladimir Palant, 2014-12-17 19:42:50]

I realized that there the ext API already defines a way to send a message back
to the sender - via sender.page.sendMessage(). So I've adjusted the emulation
here accordingly, the only addition is the Page.equals() method. This one is
required for the cleanup functionality which is implemented now - listeners in
the background page have to be cleared when the page unloads. Note that I still
consider it questionable that the cleanup message should be sent by the
abstraction layer: the abstraction layer cannot process it on the receiving end
as it doesn't know where the Page object is being used.

[Sebastian Noack, 2014-12-18 12:58:39]

On 2014/12/17 19:42:50, Wladimir Palant wrote:
> So I've adjusted the emulation here accordingly, the only addition is the
> Page.equals() method. This one is required for the cleanup functionality

The reason there isn't an Page.equals() in the abstraction layer yet, is that we
use PageMap() objects for stuff like that, where items are automatically removed
when the page is closed or navigation occurred. I strongly prefer to follow the
same approach here.

> listeners in the background page have to be cleared when the page unloads.

I remember that the "unload" event wasn't dispatched when the tab is closed (but
only when navigation occurs) on Safari. So I ended up doing the cleanup for the
background page proxy in the background page when tab's "close" event is
triggered. I am not sure about Chrome and Firefox, but the behavior might be
similar.

> Note that I still consider it questionable that the cleanup message should be
sent by the
> abstraction layer: the abstraction layer cannot process it on the receiving
end
> as it doesn't know where the Page object is being used.

I completely agree. The abstraction layer shouldn't send messages (from the
content script) that needs to be handled by high-level code (in the background
page). IMO, this is the result of a leaky abstraction; "subscriptions.listen"
abstracts FilterNotifier.addListener(). However, the underlying API requires to
call removeListener() as well, once the listener isn't needed anymore. But that
has been ignored in the messaging code.

Maybe it would be a better idea, to don't register event listeners via
messaging. Instead we could always register those event listeners and just
dispatch a message to all pages (as done by chrome.runtime.sendMessage)
every-time the event is triggered.

[Thomas Greiner, 2014-12-18 13:27:49]

On 2014/12/18 12:58:39, Sebastian Noack wrote:
> I completely agree. The abstraction layer shouldn't send messages (from the
> content script) that needs to be handled by high-level code (in the background
> page).

It's about determining when an endpoint disconnects. This can be determined by
the background page even without a message being sent (i.e. like you said you
did in Safari).

> IMO, this is the result of a leaky abstraction; "subscriptions.listen"
> abstracts FilterNotifier.addListener(). However, the underlying API requires
to
> call removeListener() as well, once the listener isn't needed anymore. But
that
> has been ignored in the messaging code.
> 
> Maybe it would be a better idea, to don't register event listeners via
> messaging. Instead we could always register those event listeners and just
> dispatch a message to all pages (as done by chrome.runtime.sendMessage)
> every-time the event is triggered.

No, we want to avoid sending too many useless messages. However, it's sufficient
to only have one single FilterNotifier listener in the background page instead
of for each "subscription.listen" message.

[Thomas Greiner, 2014-12-18 17:32:05]

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
File background.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
background.js:20: function getURLParameters(data)
The function name is not reflecting what it does (same as noted here
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/...)

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
background.js:137: var result = {platform: "gecko", platformVersion: "34.0",
application: "firefox", applicationVersion: "34.0"};
Again a line that doesn't need to be that long.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
File ext/common.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/common.js:50: listener.__wrapper = function(event)
Two underscores are easy to get wrong and detect when looking for errors. It
should be sufficient to only use a single one and to make the name more specific
(e.g. "_extWrapper").

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
File ext/content.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/content.js:25: backgroundFrame.style.visibility = "hidden";
Rather use `backgroundFrame.style.display = "none";` to also exclude it from the
browser's page layout calculation.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/content.js:75: window.addEventListener("unload", function()
I don't see why an "unload" handler make sense in this test environment since
the page (which includes the background page as well) is about to close.

[Wladimir Palant, 2014-12-18 22:04:44]

On 2014/12/18 12:58:39, Sebastian Noack wrote:
> Maybe it would be a better idea, to don't register event listeners via
> messaging. Instead we could always register those event listeners and just
> dispatch a message to all pages (as done by chrome.runtime.sendMessage)
> every-time the event is triggered.

That could be a huge number of messages, especially with filter hit counting
activated (i.e. on Firefox). Also keep in mind that normally at most one or two
pages should receive these messages (first-run and options), so broadcasting
would massively waste resources.

Unfortunately, using a PageMap here doesn't work either - PageMap doesn't have a
way to list existing keys (just like WeakMap), and adding this functionality
isn't trivial either (it only stores page IDs on Chrome and Safari, getting a
Page object from that is possible in Chrome but not in Safari).

I ended up adding an ext.onPageRemoved event target. The parameter isn't the
page itself (we cannot create a Page object, and comparing it would be
complicated anyway) but a comparator that will return true when called with a
page that is no longer valid. This is good enough to allow removing outdated
listeners.

While the "unload" event is being used to trigger ext.onPageRemoved in the test
environment (I assume that it will be the same on Firefox), on Chrome and Safari
I made ext._removeFromAllPageMaps dispatch it. This solves the issues mentioned
by Sebastian. Still rather hacky but better than a message sent by the
abstraction layer.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
File background.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
background.js:20: function getURLParameters(data)
On 2014/12/18 17:32:06, Thomas Greiner wrote:
> The function name is not reflecting what it does (same as noted here
>
http://codereview.adblockplus.org/6180766664884224/diff/5741031244955648/ext/...)

Fixed by rebasing.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/back...
background.js:137: var result = {platform: "gecko", platformVersion: "34.0",
application: "firefox", applicationVersion: "34.0"};
On 2014/12/18 17:32:06, Thomas Greiner wrote:
> Again a line that doesn't need to be that long.

Done.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
File ext/common.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/common.js:50: listener.__wrapper = function(event)
On 2014/12/18 17:32:06, Thomas Greiner wrote:
> Two underscores are easy to get wrong and detect when looking for errors. It
> should be sufficient to only use a single one and to make the name more
specific
> (e.g. "_extWrapper").

Done.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
File ext/content.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/content.js:25: backgroundFrame.style.visibility = "hidden";
On 2014/12/18 17:32:06, Thomas Greiner wrote:
> Rather use `backgroundFrame.style.display = "none";` to also exclude it from
the
> browser's page layout calculation.

Right, I forgot that frames will still load despite display:none.

http://codereview.adblockplus.org/4731979438227456/diff/5750085036015616/ext/...
ext/content.js:75: window.addEventListener("unload", function()
On 2014/12/18 17:32:06, Thomas Greiner wrote:
> I don't see why an "unload" handler make sense in this test environment since
> the page (which includes the background page as well) is about to close.

Happens to be good enough for testing, the message arrives before the
"background page" goes away and the results can be logged there.

[Sebastian Noack, 2014-12-19 13:26:19]

On 2014/12/18 22:04:44, Wladimir Palant wrote:
> On 2014/12/18 12:58:39, Sebastian Noack wrote:
> > Maybe it would be a better idea, to don't register event listeners via
> > messaging. Instead we could always register those event listeners and just
> > dispatch a message to all pages (as done by chrome.runtime.sendMessage)
> > every-time the event is triggered.
> 
> That could be a huge number of messages, especially with filter hit counting
> activated (i.e. on Firefox). Also keep in mind that normally at most one or
two
> pages should receive these messages (first-run and options), so broadcasting
> would massively waste resources.

That is a valid point. :( Though it would simplify everything a lot. :/

> Unfortunately, using a PageMap here doesn't work either - PageMap doesn't have
a
> way to list existing keys (just like WeakMap), and adding this functionality
> isn't trivial either (it only stores page IDs on Chrome and Safari, getting a
> Page object from that is possible in Chrome but not in Safari).

Adding that functionality would be trivial. On Chrome you can just create a Page
object from a tab id |new Page({id: tabId})|. On Safari pages are singletons
stored in the "pages" variable. So its a simple as |pages[pageId|.

> I ended up adding an ext.onPageRemoved event target. The parameter isn't the
> page itself (we cannot create a Page object

Again, we can.

> and comparing it would be complicated anyway)

At least on Chrome and Safari it is trivial. You can just compare the page ID. I
don't know how it works on Firefox. But anyway, I don't get why using a
comperator function instead would be any simpler.

> but a comparator that will return true when called with a
> page that is no longer valid. This is good enough to allow removing outdated
> listeners.

I still prefer to just use PageMap objects (if necessary adding a getAll()
function), instead blowing up the abstraction layer with additional and
redundant functionality.

> While the "unload" event is being used to trigger ext.onPageRemoved in the
test
> environment (I assume that it will be the same on Firefox), on Chrome and
Safari
> I made ext._removeFromAllPageMaps dispatch it. This solves the issues
mentioned
> by Sebastian. Still rather hacky but better than a message sent by the
> abstraction layer.

What's about the popup? For Kitt, and I suppose for Firefox (e10s) as well, we
also have to use asynchronous messaging there. However, the popup isn't a tab,
on Chrome, Kitt and Safari at least.

[Wladimir Palant, 2014-12-19 15:44:57]

Looks like I don't know the abstraction layer well-enough. The listeners work
via PageMap now, merely added a keys() method there. As discussed on IRC, the
pop-up will be an issue once we start using messaging there as well. However, we
can deal with it later.

[Thomas Greiner, 2014-12-19 16:58:23]

http://codereview.adblockplus.org/4731979438227456/diff/5759778777202688/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5759778777202688/mess...
messageResponder.js:89: parseInt(info.platformVersion, 10) < 6 || // beforeload
breaks websites in Safari 5
Since this is only running in the context of the background page you could use
`Services.vc.compare()` here. That of course requires a mock implementation for
the test environment (e.g. using `parseFloat()`).

[Wladimir Palant, 2014-12-19 17:45:37]

http://codereview.adblockplus.org/4731979438227456/diff/5759778777202688/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5759778777202688/mess...
messageResponder.js:89: parseInt(info.platformVersion, 10) < 6 || // beforeload
breaks websites in Safari 5
On 2014/12/19 16:58:23, Thomas Greiner wrote:
> Since this is only running in the context of the background page you could use
> `Services.vc.compare()` here. That of course requires a mock implementation
for
> the test environment (e.g. using `parseFloat()`).

Done.

[Thomas Greiner, 2014-12-19 17:59:49]

LGTM

[Sebastian Noack, 2014-12-22 09:56:16]

http://codereview.adblockplus.org/4731979438227456/diff/5636470266134528/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5636470266134528/mess...
messageResponder.js:105: global.openOptions();
window.openOptions() is deprecated. Please use ext.openOptions(). And while
implementing some parts of the abstraction layer for Firefox, you might want to
implement ext.openOptions() as well, and get rid of the switch above.

[Wladimir Palant, 2015-01-15 13:03:22]

http://codereview.adblockplus.org/4731979438227456/diff/5636470266134528/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/4731979438227456/diff/5636470266134528/mess...
messageResponder.js:105: global.openOptions();
On 2014/12/22 09:56:17, Sebastian Noack wrote:
> window.openOptions() is deprecated. Please use ext.openOptions(). And while
> implementing some parts of the abstraction layer for Firefox, you might want
to
> implement ext.openOptions() as well, and get rid of the switch above.

I filed #1813 and #1814 on that.