Issue 1587 - Escape curly brackets for elemhide filters

Issue 1587 - Escape curly brackets for elemhide filters

[Wladimir Palant, 2014-11-20 14:33:05]

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
File qunit/tests/cssEscaping.js (right):

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
qunit/tests/cssEscaping.js:22: this.filterClasses = require("filterClasses");
As with the other review, no point having this in the test setup - you can
require the module at the top of the file.

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
qunit/tests/cssEscaping.js:71: ok(false, opts.selector + " (not allowed in
elemhide filters)");
There are very few cases where code like this is justified:

  if (foo)
    ok(false);

Normally, you just check foo with qUnit:

  ok(filter instanceof ElemHideFilter, opts.selector + " allows in elemhide
filters");

[Sebastian Noack, 2014-11-20 15:47:27]

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
File qunit/tests/cssEscaping.js (right):

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
qunit/tests/cssEscaping.js:22: this.filterClasses = require("filterClasses");
On 2014/11/20 14:33:05, Wladimir Palant wrote:
> As with the other review, no point having this in the test setup - you can
> require the module at the top of the file.

Done, using an IFEE.

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
qunit/tests/cssEscaping.js:71: ok(false, opts.selector + " (not allowed in
elemhide filters)");
On 2014/11/20 14:33:05, Wladimir Palant wrote:
> There are very few cases where code like this is justified:
> 
>   if (foo)
>     ok(false);
> 
> Normally, you just check foo with qUnit:
> 
>   ok(filter instanceof ElemHideFilter, opts.selector + " allows in elemhide
> filters");

So far testSelector() only generates one assert per call. And since it is called
1540 times I prefer to keep it that way. All you need to know is whether the
selector is valid CSS, and whether it is compatible with elemhide filters. And
the message we log here provides those information.

[Wladimir Palant, 2014-11-20 17:01:20]

LGTM

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
File qunit/tests/cssEscaping.js (right):

http://codereview.adblockplus.org/5989801094283264/diff/5649050225344512/quni...
qunit/tests/cssEscaping.js:71: ok(false, opts.selector + " (not allowed in
elemhide filters)");
On 2014/11/20 15:47:27, Sebastian Noack wrote:
> So far testSelector() only generates one assert per call. And since it is
called
> 1540 times I prefer to keep it that way.

Not sure I agree but not really that important...