Issue 3860 - Move request blocking logic to a seperate module

Issue 3860 - Move request blocking logic to a seperate module

[Sebastian Noack, 2016-03-23 14:02:07]

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:116: FilterNotifier.addListener((action, arg) =>
I hope you don't mind that I revisited the logic to detect relevant filter
changes, since it turned out that both the logic in webrequest.js as well in
whitelisting.js didn't consider some cases, and even worse it was inconsistent.
So I used the chance to fix that.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:87: port.on("filters.isPageWhitelisted", (message, sender)
=>
This is unrelated, but I moved this function up here just below the function
that is called, in order to improve the code locality,

[kzar, 2016-03-23 15:36:00]

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:86: let sitekey = getKey(sender.page, sender.frame);
Nit: the name `sitekey` seems inconsistent with the name `key` used above in the
request listener.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:104: if (!(filter instanceof BlockingFilter))
I think it would be clearer to check if the filter is an instance of
BlockingFilter, removing the need for the continue. Something like this

if (filter instanceof BlockingFilter)
{
  blocked = true;
  if (filter.collapse != null)
    return filter.collapse;
}

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:116: FilterNotifier.addListener((action, arg) =>
On 2016/03/23 14:02:06, Sebastian Noack wrote:
> I hope you don't mind that I revisited the logic to detect relevant filter
> changes, since it turned out that both the logic in webrequest.js as well in
> whitelisting.js didn't consider some cases, and even worse it was
inconsistent.
> So I used the chance to fix that.

Acknowledged.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:118: // Handler behavior is already going to be changed.
We do so asynchronously, to
Nit: Mind wrapping this long line?

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:118: // Handler behavior is already going to be changed.
We do so asynchronously, to
I don't really understand this comment. Maybe something like this instead?

"Avoid triggering filters.behaviorChanged multiple times when multiple filter
changes happen at the same time."

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:87: port.on("filters.isPageWhitelisted", (message, sender)
=>
On 2016/03/23 14:02:06, Sebastian Noack wrote:
> This is unrelated, but I moved this function up here just below the function
> that is called, in order to improve the code locality,

Acknowledged. I can also see an argument for keeping all message listeners
together at the bottom of modules though. I leave it up to you anyway.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:143: let key = parts[0].replace(/=/g, "");
Don't we rather want to trim the "="s from the start of the key than remove them
throughout the first part?

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:175: chrome.webRequest.onHeadersReceived.addListener(
I guess this part should be in ext/chrome somewhere?

[Sebastian Noack, 2016-03-23 16:44:48]

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:86: let sitekey = getKey(sender.page, sender.frame);
On 2016/03/23 15:36:00, kzar wrote:
> Nit: the name `sitekey` seems inconsistent with the name `key` used above in
the
> request listener. 

Well, it was inconsistent before. But fine with me using sitekey (like in other
modules) everywhere.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:104: if (!(filter instanceof BlockingFilter))
On 2016/03/23 15:35:59, kzar wrote:
> I think it would be clearer to check if the filter is an instance of
> BlockingFilter, removing the need for the continue. Something like this
> 
> if (filter instanceof BlockingFilter)
> {
>   blocked = true;
>   if (filter.collapse != null)
>     return filter.collapse;
> }

I'm not sure whether I agree, that this is any easier to understand as it
requires another block level, but done.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/requestBlocker....
lib/requestBlocker.js:118: // Handler behavior is already going to be changed.
We do so asynchronously, to
On 2016/03/23 15:35:59, kzar wrote:
> I don't really understand this comment. Maybe something like this instead?
> 
> "Avoid triggering filters.behaviorChanged multiple times when multiple filter
> changes happen at the same time."

Done.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:143: let key = parts[0].replace(/=/g, "");
On 2016/03/23 15:36:00, kzar wrote:
> Don't we rather want to trim the "="s from the start of the key than remove
them
> throughout the first part?

We want to trim them from the end. However, they can only appear at the end in
base64 anyway. So this simpler regexp is saved. Note that we also used it
before.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:175: chrome.webRequest.onHeadersReceived.addListener(
On 2016/03/23 15:36:00, kzar wrote:
> I guess this part should be in ext/chrome somewhere?

Not really. There is no counterpart for Safari. And we generally don't add APIs
that only exist on one platform to the abstraction layer.

[kzar, 2016-03-23 17:18:22]

LGTM

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:143: let key = parts[0].replace(/=/g, "");
On 2016/03/23 16:44:48, Sebastian Noack wrote:
> On 2016/03/23 15:36:00, kzar wrote:
> > Don't we rather want to trim the "="s from the start of the key than remove
> them
> > throughout the first part?
> 
> We want to trim them from the end. However, they can only appear at the end in
> base64 anyway. So this simpler regexp is saved. Note that we also used it
> before.

Acknowledged.

https://codereview.adblockplus.org/29338962/diff/29338963/lib/whitelisting.js...
lib/whitelisting.js:175: chrome.webRequest.onHeadersReceived.addListener(
On 2016/03/23 16:44:48, Sebastian Noack wrote:
> On 2016/03/23 15:36:00, kzar wrote:
> > I guess this part should be in ext/chrome somewhere?
> 
> Not really. There is no counterpart for Safari. And we generally don't add
APIs
> that only exist on one platform to the abstraction layer.

Acknowledged.