Issue 616 - Enforce $generichide and $genericblock in Chrome

Issue 616 - Enforce $generichide and $genericblock in Chrome

Depends on these changes to adblockplus:
http://codereview.adblockplus.org/5840485868371968/ http://codereview.adblockplus.org/5991150368325632/

and these changes to adblockplustests:
https://codereview.adblockplus.org/6439460933730304/

[Sebastian Noack, 2015-03-12 12:36:12]

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/back...
background.js:474: var specificOnly = isFrameWhitelisted(sender.page,
sender.frame, "GENERICHIDE");
I'm concerned about the performance implications running yet another filter
match here. We already have serious issues with the performance of this code.

Currently we look for "DOCUMENT" exception rules, if there are none we look for
"ELEMHIDE" exception rules, and if there are still none we now also look for
"GENERICHIDE" exception rules.

So how about optimizing Matcher.matchesAny() to consider multiple types? The
algorithm could look roughly like this:

function matchesAny(types)
{
  result = null;
  priority = types.length - 1;

  for (filter of filters)
  {
    for (i = 0; i <= priority; i++)
    {
      if (filter.matches(types[i]))
      {
         result = filter;
         priority = i;
      }
    }

    if (priority == 0)
      break;
  }

  return filter;
}

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/webr...
File webrequest.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/webr...
webrequest.js:64: specificOnly
I suppose this isn't the only call that needs to be adapted. What's about
popupBlocker.js?

[kzar, 2015-03-12 19:22:45]

Patch Set 2 : Consider $genericblock when blocking popups.

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/back...
background.js:474: var specificOnly = isFrameWhitelisted(sender.page,
sender.frame, "GENERICHIDE");
On 2015/03/12 12:36:12, Sebastian Noack wrote:
> I'm concerned about the performance implications running yet another filter
> match here. We already have serious issues with the performance of this code.
> 
> Currently we look for "DOCUMENT" exception rules, if there are none we look
for
> "ELEMHIDE" exception rules, and if there are still none we now also look for
> "GENERICHIDE" exception rules.
> 
> So how about optimizing Matcher.matchesAny() to consider multiple types? The
> algorithm could look roughly like this:
> 
> function matchesAny(types)
> {
>   result = null;
>   priority = types.length - 1;
> 
>   for (filter of filters)
>   {
>     for (i = 0; i <= priority; i++)
>     {
>       if (filter.matches(types[i]))
>       {
>          result = filter;
>          priority = i;
>       }
>     }
> 
>     if (priority == 0)
>       break;
>   }
> 
>   return filter;
> }

That sound sensible to me but as you mentioned to me on IRC this would require a
new (blocking) issue to be filed and fixed to adapt matchesAny first.

What's your opinion here Wladimir?

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/webr...
File webrequest.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5629499534213120/webr...
webrequest.js:64: specificOnly
On 2015/03/12 12:36:12, Sebastian Noack wrote:
> I suppose this isn't the only call that needs to be adapted. What's about
> popupBlocker.js?

Good point, I didn't consider popups! I updated my test page and it turns out my
Firefox changes work fine but these Chrome ones don't. Fixed that now.

Done.

[kzar, 2015-03-12 19:47:27]

Patch Set 3 : Removed redundant logic in popup blocking changes.

[Sebastian Noack, 2015-03-12 19:59:36]

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
File popupBlocker.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
popupBlocker.js:62: function checkPotentialPopup(tabId, url, specificOnly,
documentHost)
Nit: I'd put the documentHost first to match the signature of matchesAny().

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
popupBlocker.js:69: undefined, specificOnly
Nit: Please null instead undefined.

[kzar, 2015-03-12 20:42:12]

Patch Set 5 : Addressed feedback.

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
File popupBlocker.js (right):

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
popupBlocker.js:62: function checkPotentialPopup(tabId, url, specificOnly,
documentHost)
On 2015/03/12 19:59:36, Sebastian Noack wrote:
> Nit: I'd put the documentHost first to match the signature of matchesAny().

Done.

http://codereview.adblockplus.org/5138680696012800/diff/5657382461898752/popu...
popupBlocker.js:69: undefined, specificOnly
On 2015/03/12 19:59:36, Sebastian Noack wrote:
> Nit: Please null instead undefined.

Done.

[Sebastian Noack, 2015-03-12 20:44:28]

Looks good, beside my performance concerns of course.

[kzar, 2015-07-14 16:56:28]

Patch Set 6 : Rebased onto typeMask changes

[Felix Dahlke, 2015-08-20 10:15:07]

Same as in https://codereview.adblockplus.org/5840485868371968/ - moving
Wladimir to CC, adding Thomas and me as reviewers.

[kzar, 2015-08-26 10:21:16]

Patch Set 7 : Obey 80 character line limit

[Thomas Greiner, 2015-09-02 16:28:21]

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
popupBlocker.js:54: var source = tabsLoading[tabId];
Would make sense to check for `tabId in tabsLoading` here.

Currently, it appears to be simply passed on so in case it was `undefined` it
doesn't cause any issues. However, with this change we're going to try to access
properties on it which will throw an exception if "source" is `undefined`.

[kzar, 2015-09-05 14:53:44]

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
popupBlocker.js:54: var source = tabsLoading[tabId];
On 2015/09/02 16:28:20, Thomas Greiner wrote:
> Would make sense to check for `tabId in tabsLoading` here.
> 
> Currently, it appears to be simply passed on so in case it was `undefined` it
> doesn't cause any issues. However, with this change we're going to try to
access
> properties on it which will throw an exception if "source" is `undefined`.

I think that check is already carried out just above.

[Thomas Greiner, 2015-09-18 17:41:06]

LGTM

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
File popupBlocker.js (right):

https://codereview.adblockplus.org/5138680696012800/diff/29324652/popupBlocke...
popupBlocker.js:54: var source = tabsLoading[tabId];
On 2015/09/05 14:53:44, kzar wrote:
> On 2015/09/02 16:28:20, Thomas Greiner wrote:
> > Would make sense to check for `tabId in tabsLoading` here.
> > 
> > Currently, it appears to be simply passed on so in case it was `undefined`
it
> > doesn't cause any issues. However, with this change we're going to try to
> access
> > properties on it which will throw an exception if "source" is `undefined`.
> 
> I think that check is already carried out just above.

Don't know how I missed that. You're right.

[Sebastian Noack, 2015-09-21 14:25:09]

LGTM

[Felix Dahlke, 2015-09-28 08:28:52]

LGTM

[kzar, 2015-09-28 17:21:49]

Patch Set 8 : Rebased and updated adblockplus dependency

We still need to wait for the adblockplustests changes to be pushed so we can
update that dependency too.

[kzar, 2015-10-05 09:56:57]

Patch Set 9 : Updated adblockplustests dependency

(Hopefully the last change! If this gets the OK we're ready to finally push.)

[Sebastian Noack, 2015-10-05 10:44:12]

LGTM

[Felix Dahlke, 2015-10-05 11:37:31]

LGTM