Added abstraction for frames, to fix domain-based rules, whitelisting and ad counter on Safari

In order to fix whitelisting for Safari, I have added an abstraction layer for frames. So event handlers for ext.webRequest.onBeforeload and ext.onMessage, receive a Frame object, instead of Chrome-specific frame ids, now. Also I had to implement key-based whitelisting via HTML attribute, because of in Safari there is no way to read response headers. This patch is huge since I had to change all code that relied on frames.

Also I had to use the onBeforeNavigate event in order to make thing work cross Chrome/Safari. But this event belongs to the webNavigation API on Chromium. So I broke compatibility with Opera 15-16. Please let me know whether whether we can drop compatibility for Opera 15 and 16 (18 is current stable). Otherwise I could easily implement a fallback based on content scripts and the beforeunload event.

[Sebastian Noack, 2013-12-21 20:19:16]

In order to fix whitelisting for Safari, I have added an abstraction layer for
frames. So event handlers for ext.webRequest.onBeforeload and ext.onMessage,
receive a Frame object, instead of Chrome-specific frame ids, now. Also I had to
implement key-based whitelisting via HTML attribute, because of in Safari there
is no way to read response headers. This patch is huge since I had to change all
code that relied on frames.

Also I had to use the onBeforeNavigate event in order to make thing work cross
Chrome/Safari. But this event belongs to the webNavigation API on Chromium. So I
broke compatibility with Opera 15-16. Please let me know whether whether we can
drop compatibility for Opera 15 and 16 (18 is current stable). Otherwise I could
easily implement a fallback based on content scripts and the beforeunload event.

[Wladimir Palant, 2014-01-18 02:46:35]

This seems to be the wrong approach and unnecessarily complicating everything.
The natural approach would be storing a frame ID for each frame in the Safari
content script. Top-level frames have frame ID 0, any other frame gets assigned
a fresh ID in the response to the first webRequest message.

[Felix Dahlke, 2014-01-18 13:39:19]

Quite a large patch indeed. Anyway, looks good to me, some of my traditional nit
picking below. I think some unrelated changes made it into the diff, commented
on those as well.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:417: if (frame)
This would return undefined if frame is falsy. I think something like this would
be simpler:

if (!frame || frame.parent == -1)
  return null;
return new Frame({id: frame.parent, tab: this._tab});

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:433: // them here instead of in the browser independant
high-level code.
s/independant/independent/

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:469: // frame was loaded, the frame is unkown and we
have to ignore the request.
s/unkown/unknown/

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:477: if
(ext.webRequest.onBeforeRequest._listeners[i](details.url, details.type, tab,
frame) === false)
Why not the following?

if (!ext.webRequest.onBeforeRequest._listeners[i](details.url, details.type
     , tab, frame))

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
File lib/stats.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
lib/stats.js:41: return tabStats && tabStats.blocked || 0;
We're typically not using and/or as branching operators in JavaScript, that's
what ?: is for.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
lib/whitelisting.js:56: if (!defaultMatcher.matchesByKey(url, match[2],
docDomain))
I think this would be easier to follow if you'd assign the matches to
temporaries. match[2] as "key" etc.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/background.js:188: setTimeout(this._delete.bind(this, event.target),
0);
s/lookup this tab/look this tab up/

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/background.js:235: // this is the best we can do for Safari. :(
We could cache a chain of frames like we do in Android. It's just an
approximation, but better for whitelisting. That's something for a follow-up
though, this is fine for now.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/common.js:75:
this._getResponseDispatcher(event).dispatchMessage("response", {
{ should go on its own line

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/content.js:312: if (!safari.self.tab.canLoad(event, {
{ should go on its own line

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/content.js:328: sendMessage: function(message, responseCallback) {
{ should go on its own line

[Sebastian Noack, 2014-01-19 10:19:28]

On 2014/01/18 02:46:35, Wladimir Palant wrote:
> This seems to be the wrong approach and unnecessarily complicating everything.
> The natural approach would be storing a frame ID for each frame in the Safari
> content script. Top-level frames have frame ID 0, any other frame gets
assigned
> a fresh ID in the response to the first webRequest message.

I don't see how that would make anything less complicated. The solution for
Safari is as simple as checking whether the current frame is the top level frame
and if not, assuming that the parent frame is the top level frame. I also
considered emulating frame IDs on Safari. But since you have to set them in the
content script, you either have to use a random numbers for frame IDs or need
additional synchronous messaging to synchronize them with the background page.
If you think that adding more abstraction, is what makes it more complicated, it
doesn't. That way Chrome-specific workarounds (like looking up parent frames
from within onMessage, where you only have the frame's URL) stay into the
Chrome-specifc code, and Safari-specific workarounds stay into Safari-specific
code. In fact not much complexity is added, existing complexity just got moved
into ext.

[Sebastian Noack, 2014-01-19 10:19:40]

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:417: if (frame)
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> This would return undefined if frame is falsy. I think something like this
would
> be simpler:
> 
> if (!frame || frame.parent == -1)
>   return null;
> return new Frame({id: frame.parent, tab: this._tab});

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:433: // them here instead of in the browser independant
high-level code.
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> s/independant/independent/

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:469: // frame was loaded, the frame is unkown and we
have to ignore the request.
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> s/unkown/unknown/

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/chro...
chrome/ext/background.js:477: if
(ext.webRequest.onBeforeRequest._listeners[i](details.url, details.type, tab,
frame) === false)
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> Why not the following?
> 
> if (!ext.webRequest.onBeforeRequest._listeners[i](details.url, details.type
>      , tab, frame))

In order to block the request, the handler should explicitly return false (like
the old way to cancel DOM events). When the function doesn't explicitly return,
the return value is undefined in which case we don't want to block the request.
So I'm using the === operator to make sure that the return value is false (and
not just null or undefined). Also that approach isn't new, it was already used
before this patchset.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
File lib/stats.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
lib/stats.js:41: return tabStats && tabStats.blocked || 0;
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> We're typically not using and/or as branching operators in JavaScript, that's
> what ?: is for.

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/lib/...
lib/whitelisting.js:56: if (!defaultMatcher.matchesByKey(url, match[2],
docDomain))
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> I think this would be easier to follow if you'd assign the matches to
> temporaries. match[2] as "key" etc.

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/background.js:188: setTimeout(this._delete.bind(this, event.target),
0);
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> s/lookup this tab/look this tab up/

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/common.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/common.js:75:
this._getResponseDispatcher(event).dispatchMessage("response", {
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> { should go on its own line

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
File safari/ext/content.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/content.js:312: if (!safari.self.tab.canLoad(event, {
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> { should go on its own line

Done.

http://codereview.adblockplus.org/6346177440120832/diff/5629499534213120/safa...
safari/ext/content.js:328: sendMessage: function(message, responseCallback) {
On 2014/01/18 13:39:19, Felix H. Dahlke wrote:
> { should go on its own line

Done.

[Felix Dahlke, 2014-01-20 00:02:14]

LGTM, one final style nit.

http://codereview.adblockplus.org/6346177440120832/diff/4601851299233792/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6346177440120832/diff/4601851299233792/chro...
chrome/ext/background.js:419: else
No else before return, as described in the Mozilla Coding Style.