Added handling of whitelisting rules like @@||example.com^$document

Added handling of whitelisting rules like @@||example.com^$document

[Felix Dahlke, 2012-11-14 13:03:50]

Found two small issues.

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js
File lib/matcher.js (right):

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode218
lib/matcher.js:218: if (!WhitelistFilter)
Not a new problem, but can't we do this earlier? Both remove and _generateRule
rely on add being executed first. (Arguably less of a problem with
_generateRule, since it's only invoked from add.)

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode224
lib/matcher.js:224: if (filter instanceof WhitelistFilter && filter.contentType
== RegExpFilter.typeMap.DOCUMENT)
I'd like a function for both this check and the _getDomainFromException call
below, since we have the same code in remove.

Maybe something like this:

var whitelistedDomain = this._getWhitelistedDomain(filter);
if (whitelistedDomain)
  this._addDomainException(whitelistedDomain); // or remove, respectively

[Wladimir Palant, 2012-11-14 13:46:53]

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js
File lib/matcher.js (right):

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode218
lib/matcher.js:218: if (!WhitelistFilter)
On 2012/11/14 13:03:51, Felix H. Dahlke wrote:
> Not a new problem, but can't we do this earlier? Both remove and _generateRule
> rely on add being executed first. (Arguably less of a problem with
> _generateRule, since it's only invoked from add.)

Yes, it's ugly. Problem is that matcher.js needs to be included before
background.js - meaning that it cannot just require() its dependencies while
loading. Let's try a different hack...

http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode224
lib/matcher.js:224: if (filter instanceof WhitelistFilter && filter.contentType
== RegExpFilter.typeMap.DOCUMENT)
Ok, I changed the solution to have maximal code reuse.

[Felix Dahlke, 2012-11-14 16:18:34]

On 2012/11/14 13:46:53, Wladimir Palant wrote:
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js
> File lib/matcher.js (right):
> 
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode218
> lib/matcher.js:218: if (!WhitelistFilter)
> On 2012/11/14 13:03:51, Felix H. Dahlke wrote:
> > Not a new problem, but can't we do this earlier? Both remove and
_generateRule
> > rely on add being executed first. (Arguably less of a problem with
> > _generateRule, since it's only invoked from add.)
> 
> Yes, it's ugly. Problem is that matcher.js needs to be included before
> background.js - meaning that it cannot just require() its dependencies while
> loading. Let's try a different hack...
> 
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode224
> lib/matcher.js:224: if (filter instanceof WhitelistFilter &&
filter.contentType
> == RegExpFilter.typeMap.DOCUMENT)
> Ok, I changed the solution to have maximal code reuse.

LGTM

[Felix Dahlke, 2012-11-14 16:18:34]

On 2012/11/14 13:46:53, Wladimir Palant wrote:
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js
> File lib/matcher.js (right):
> 
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode218
> lib/matcher.js:218: if (!WhitelistFilter)
> On 2012/11/14 13:03:51, Felix H. Dahlke wrote:
> > Not a new problem, but can't we do this earlier? Both remove and
_generateRule
> > rely on add being executed first. (Arguably less of a problem with
> > _generateRule, since it's only invoked from add.)
> 
> Yes, it's ugly. Problem is that matcher.js needs to be included before
> background.js - meaning that it cannot just require() its dependencies while
> loading. Let's try a different hack...
> 
> http://codereview.adblockplus.org/8790183/diff/1/lib/matcher.js#newcode224
> lib/matcher.js:224: if (filter instanceof WhitelistFilter &&
filter.contentType
> == RegExpFilter.typeMap.DOCUMENT)
> Ok, I changed the solution to have maximal code reuse.

LGTM