Issue 2738 - Pass bit masks to matching functions

Issue 2738 - Pass bit masks to matching functions

(Depends on changes in https://codereview.adblockplus.org/29321478/ )

I have tested to best of my ability but not sure how to verify everything is OK for sure.

[Sebastian Noack, 2015-07-09 15:28:52]

https://codereview.adblockplus.org/29321504/diff/29321505/background.js
File background.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/background.js#newco...
background.js:344: stringifyURL(url), RegExpFilter.toTypeMask(msg.mediatype),
This operation can be done outside of the loop.

https://codereview.adblockplus.org/29321504/diff/29321505/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/lib/filterComposer....
lib/filterComposer.js:102: url, RegExpFilter.toTypeMask(details.type),
docDomain,
Same here:

  let docDomain = extractHostFromFrame(frame);
  let typeMask = RegExpFilter.toTypeMask(details.type);

  for (let url of details.urls)
  {
     ..
     let filter = defaultMatcher.whitelist.matchesAny(
        url, typeMask, docDomain,
        isThirdParty(urlObj, docDomain),
        getKey(page, frame)
      );

https://codereview.adblockplus.org/29321504/diff/29321505/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/lib/whitelisting.js...
lib/whitelisting.js:60: stringifyURL(url), RegExpFilter.toTypeMask(type ||
"DOCUMENT"),
And here as well.

[Sebastian Noack, 2015-07-09 15:40:25]

Since the whole purpose of this change is to check several request types at
once. So the isPageWhistelisted and isFrameWhitelisted function should simply
pass through the type mask.

[kzar, 2015-07-12 14:28:01]

Patch Set 2 : Updated to match adblockplus changes and addressed feedback

https://codereview.adblockplus.org/29321504/diff/29321505/background.js
File background.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/background.js#newco...
background.js:344: stringifyURL(url), RegExpFilter.toTypeMask(msg.mediatype),
On 2015/07/09 15:28:52, Sebastian Noack wrote:
> This operation can be done outside of the loop.

Done.

https://codereview.adblockplus.org/29321504/diff/29321505/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/lib/filterComposer....
lib/filterComposer.js:102: url, RegExpFilter.toTypeMask(details.type),
docDomain,
On 2015/07/09 15:28:52, Sebastian Noack wrote:
> Same here:
> 
>   let docDomain = extractHostFromFrame(frame);
>   let typeMask = RegExpFilter.toTypeMask(details.type);
> 
>   for (let url of details.urls)
>   {
>      ..
>      let filter = defaultMatcher.whitelist.matchesAny(
>         url, typeMask, docDomain,
>         isThirdParty(urlObj, docDomain),
>         getKey(page, frame)
>       );

Done.

https://codereview.adblockplus.org/29321504/diff/29321505/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29321504/diff/29321505/lib/whitelisting.js...
lib/whitelisting.js:60: stringifyURL(url), RegExpFilter.toTypeMask(type ||
"DOCUMENT"),
On 2015/07/09 15:28:52, Sebastian Noack wrote:
> And here as well.

Done.

[Sebastian Noack, 2015-07-13 16:41:51]

https://codereview.adblockplus.org/29321504/diff/29322033/background.js
File background.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/background.js#newco...
background.js:339: var typeMask = RegExpFilter.typeMap[msg.mediatype];
Nit: I'd put it above the declaration of documentHost. Then you have all
variables declared in the order they are used, plus the variables passed to the
matcher will be declared in the same place.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/lib/filterComposer....
lib/filterComposer.js:94: let typeMask = RegExpFilter.typeMap[details.type];
Nit: I'd declare the variables here in the order they are used.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js...
lib/whitelisting.js:33: exports.isPageWhitelisted = function(page, typeMask)
Sorry if my previous comment was misleading. But since isPageWhitelisted() is
never called with an explicit type, there is no point in adding an argument for
that. I were actually only talking about isFrameWhitelisted(), which of course
should pass through the bitmask rather than converting a string into one.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js...
lib/whitelisting.js:48: * @param {Number}
[typeMask=RegExpFilter.typeMap.DOCUMENT]  Bit mask of request / content types to
match
Nit: Please use lowercase when referring to primitive types. Otherwise it would
be ambiguous with the Number wrapper object.

[Wladimir Palant, 2015-07-13 18:59:10]

No additional issues from my side, this looks fine.

[kzar, 2015-07-14 09:22:24]

Patch Set 3 : Addressed final nits

https://codereview.adblockplus.org/29321504/diff/29322033/background.js
File background.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/background.js#newco...
background.js:339: var typeMask = RegExpFilter.typeMap[msg.mediatype];
On 2015/07/13 16:41:50, Sebastian Noack wrote:
> Nit: I'd put it above the declaration of documentHost. Then you have all
> variables declared in the order they are used, plus the variables passed to
the
> matcher will be declared in the same place.

Done.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/lib/filterComposer....
lib/filterComposer.js:94: let typeMask = RegExpFilter.typeMap[details.type];
On 2015/07/13 16:41:50, Sebastian Noack wrote:
> Nit: I'd declare the variables here in the order they are used.

Done.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js...
lib/whitelisting.js:33: exports.isPageWhitelisted = function(page, typeMask)
On 2015/07/13 16:41:50, Sebastian Noack wrote:
> Sorry if my previous comment was misleading. But since isPageWhitelisted() is
> never called with an explicit type, there is no point in adding an argument
for
> that. I were actually only talking about isFrameWhitelisted(), which of course
> should pass through the bitmask rather than converting a string into one.

Done.

https://codereview.adblockplus.org/29321504/diff/29322033/lib/whitelisting.js...
lib/whitelisting.js:48: * @param {Number}
[typeMask=RegExpFilter.typeMap.DOCUMENT]  Bit mask of request / content types to
match
On 2015/07/13 16:41:50, Sebastian Noack wrote:
> Nit: Please use lowercase when referring to primitive types. Otherwise it
would
> be ambiguous with the Number wrapper object.

Done.

[Sebastian Noack, 2015-07-14 09:26:12]

LGTM

[kzar, 2015-07-14 14:47:13]

Patch Set 4 : Updated adblockplus dependency

[Sebastian Noack, 2015-07-14 14:49:44]

LGTM

[Wladimir Palant, 2015-07-16 11:57:21]

LGTM