Issue 2738 - Make RegExpFilter.matches() take a bit mask instead of content type string

Issue 2738 - Make RegExpFilter.matches() take a bit mask instead of content type string

[Sebastian Noack, 2015-07-09 15:36:45]

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:636: if ((this.contentType & typeMask) != 0 &&
Note that |!= 0| is unneeded here.

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:768: RegExpFilter.toTypeMask = function (contentType)
I feel that this function just adds unneeded complexity and overhead. From the
use cases I've seen so far, one could as well write..

  RegExpFilter.typeMap[mediatype]

..instead..

  RegExpFilter.toTypeMask(mediatype)

[kzar, 2015-07-09 17:44:19]

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:768: RegExpFilter.toTypeMask = function (contentType)
On 2015/07/09 15:36:44, Sebastian Noack wrote:
> I feel that this function just adds unneeded complexity and overhead. From the
> use cases I've seen so far, one could as well write..
> 
>   RegExpFilter.typeMap[mediatype]
> 
> ..instead..
> 
>   RegExpFilter.toTypeMask(mediatype)

I've got to say I agree with you here, the few times we _do_ need multiple types
the calling code could just bitwise OR the types together itself. What do you
think Wladimir?

[Wladimir Palant, 2015-07-10 21:07:01]

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.j...
lib/contentPolicy.js:122: this.defaultTypeMask = RegExpFilter.toTypeMask("");
It's a bit mask - the default is always 0.

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.j...
lib/contentPolicy.js:247: match = defaultMatcher.matchesAny(locationText,
Policy.typeMask[contentType] || Policy.defaultTypeMask, docDomain, thirdParty,
sitekey);
This cannot possibly return a match if type mask is 0, why even bother calling?
How about changing the condition above to |!match && Prefs.enabled &&
contentType in Policy.typeMask|?

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:768: RegExpFilter.toTypeMask = function (contentType)
On 2015/07/09 17:44:18, kzar wrote:
> I've got to say I agree with you here, the few times we _do_ need multiple
types
> the calling code could just bitwise OR the types together itself. What do you
> think Wladimir?

Not really sure this is a good idea but well - let's treat RegExpFilter.typeMap
like a public enum (it's already being used in a few places in the code). And -
no, having it around just to convert arrays isn't worth the effort.
RegExpFilter.typeMask.DOCUMENT definitely reads better.

[kzar, 2015-07-12 13:59:39]

Patch Set 2 : Changed API as discussed and addressed feedback

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.j...
lib/contentPolicy.js:122: this.defaultTypeMask = RegExpFilter.toTypeMask("");
On 2015/07/10 21:07:00, Wladimir Palant wrote:
> It's a bit mask - the default is always 0.

Egad, that was stupid of me.
Done.

https://codereview.adblockplus.org/29321478/diff/29321479/lib/contentPolicy.j...
lib/contentPolicy.js:247: match = defaultMatcher.matchesAny(locationText,
Policy.typeMask[contentType] || Policy.defaultTypeMask, docDomain, thirdParty,
sitekey);
On 2015/07/10 21:07:00, Wladimir Palant wrote:
> This cannot possibly return a match if type mask is 0, why even bother
calling?
> How about changing the condition above to |!match && Prefs.enabled &&
> contentType in Policy.typeMask|?

Done.

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:636: if ((this.contentType & typeMask) != 0 &&
On 2015/07/09 15:36:44, Sebastian Noack wrote:
> Note that |!= 0| is unneeded here.

Done.

https://codereview.adblockplus.org/29321478/diff/29321479/lib/filterClasses.j...
lib/filterClasses.js:768: RegExpFilter.toTypeMask = function (contentType)
On 2015/07/10 21:07:00, Wladimir Palant wrote:
> On 2015/07/09 17:44:18, kzar wrote:
> > I've got to say I agree with you here, the few times we _do_ need multiple
> types
> > the calling code could just bitwise OR the types together itself. What do
you
> > think Wladimir?
> 
> Not really sure this is a good idea but well - let's treat
RegExpFilter.typeMap
> like a public enum (it's already being used in a few places in the code). And
-
> no, having it around just to convert arrays isn't worth the effort.
> RegExpFilter.typeMask.DOCUMENT definitely reads better.

Done.

[Sebastian Noack, 2015-07-13 16:25:29]

LGTM from my side. But Wladimir should do a final review as well, since it is
his module.

[Wladimir Palant, 2015-07-13 18:55:11]

LGTM