Issue 491 - Validate custom filters

Issue 491 - Validate custom filters

[kzar, 2014-11-18 16:32:09]

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
background.js:529: 
How come you don't check that msg.filters and msg.filters.length are truthy here
any more?

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
background.js:532: var filter = Filter.fromText(msg.filters[i], true);
Instead of having this separate filter variable how about pushing the filter
onto the filters array straight away? Then you could check if filters[i] was an
instance of InvalidFilter before proceeding.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
background.js:541: for (var i = 0; i < filters.length; i++)
You've re-declared the `i` variable here.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
File options.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:478: {
Seems like the logic here is duplicated from background.js. Maybe there would be
a way to create a utility function that checks if a filter is valid that can be
reused?

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:483: return;
IMHO it would be more readable if you omitted the return on Invalid there and
had an else clause to add the filter. That way you wouldn't need the enclosing
brackets and the code would be a bit shorter.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:555: var filter = Filter.fromText(text, true);
Again seems like this logic is duplicate of above and logic in background.js

[Wladimir Palant, 2014-11-18 16:35:03]

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/bloc...
File block.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/bloc...
block.js:83: alert(response.error);
I don't think this message is helpful without indicating which filter it applies
to. Besides, showing the browser's error message in addition to our generic
description would be nice.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
File options.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:560: }
It would probably make sense to have a filter validation function somewhere
rather than repeating this code over and over again:

  try
  {
    var add = filters.map(createFilter);
    ...
  }
  catch (e)
  {
    alert(e.message);
  }

[Sebastian Noack, 2014-11-18 17:42:54]

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
File background.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
background.js:529: 
On 2014/11/18 16:32:10, kzar wrote:
> How come you don't check that msg.filters and msg.filters.length are truthy
here
> any more?

There was no point in checking it in the first place, since this message is
always send with an array, and whether it is empty or not doesn't matter.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/back...
background.js:532: var filter = Filter.fromText(msg.filters[i], true);
On 2014/11/18 16:32:10, kzar wrote:
> Instead of having this separate filter variable how about pushing the filter
> onto the filters array straight away? Then you could check if filters[i] was
an
> instance of InvalidFilter before proceeding.

We have to access the filter object 3 times (or 2 times if directly pushed to
the array). So accessing that object via an array index, is neither more
readable nor efficient.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/bloc...
File block.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/bloc...
block.js:83: alert(response.error);
On 2014/11/18 16:35:03, Wladimir Palant wrote:
> I don't think this message is helpful without indicating which filter it
applies
> to. Besides, showing the browser's error message in addition to our generic
> description would be nice.

I decided to keep things as simple as possible for now, since we are probably
going to rework the UI anyway. And at the moment any error message is better
than no error message.

Also the browser's error message is as generic as our error message, at least on
Chrome and Safari, though it mentions the selector. However, for other cases of
InvalidFilter there won't even be a browser error.

But I guess we can add the line number at least.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
File options.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:478: {
On 2014/11/18 16:32:10, kzar wrote:
> Seems like the logic here is duplicated from background.js. Maybe there would
be
> a way to create a utility function that checks if a filter is valid that can
be
> reused?

The only common code is the call to alert(). Note that we still have to check
whether the input is invalid and bail out at the right point. This is something
that a utility function can't do.

However, there will be more common code if we decide to add more details to the
shown error message (e.g. the line number) and/or if we decide to don't validate
the CSS selector in the core code, as currently discussed in
http://codereview.adblockplus.org/5772186375684096/.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:483: return;
On 2014/11/18 16:32:10, kzar wrote:
> IMHO it would be more readable if you omitted the return on Invalid there and
> had an else clause to add the filter. That way you wouldn't need the enclosing
> brackets and the code would be a bit shorter.

Then the code below which clears the field would also be reached when the filter
didn't validate.

[Sebastian Noack, 2014-11-19 12:31:38]

I've uploaded a new patchset, that does the CSS validation in the extension,
instead the core. I agreed with Wladimir that this makes more sense, mostly
because this code is platform specific.

Therfore I implemented the functions parseFilter() and parseFilters(), which
also unify some code that were previously duplicated.

Also the error messages are more verbose now, and include the line number as
well as the invalid CSS selector.

[Wladimir Palant, 2014-11-19 16:03:10]

Looks good, only one issue below.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
File options.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:553: continue;
You've removed that logic. People will occasionally use this functionality to
import a filter list. We shouldn't import the filter list header like [Adblock]
or [Adblock Plus] as a filter.

[Sebastian Noack, 2014-11-20 12:51:19]

I also added unit tests for the filter validation.

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
File options.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5629499534213120/opti...
options.js:553: continue;
On 2014/11/19 16:03:11, Wladimir Palant wrote:
> You've removed that logic. People will occasionally use this functionality to
> import a filter list. We shouldn't import the filter list header like
[Adblock]
> or [Adblock Plus] as a filter.

When I unified the code into a reusable helper function I had to decide whether
to strip filter list headers as done  here, or whether to preserve them as done
when adding a single filter or using the "Block Element".

Discarding the user input, when a single filter starting with [ where typed,
would result in bad and confusing user experience. But only stripping filter
list headers when in raw edit mode, while allowing them otherwise, would be
inconsistent.

So I am now stripping filter list headers again in raw edit mode, while showing
an error in other scenarios.

[Wladimir Palant, 2014-11-20 14:10:06]

Code is good, thank you. Unit test still needs some work however.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:13: this.CommentFilter =
filterClasses.CommentFilter;
It makes relatively little sense to require modules for each test run
individually - these won't change from one test to another and don't need to be
cleaned up. You can do that at the top of the file instead.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:18: test("detecting invalid filters", function()
{
Nit: Here and below, test names are usually capitalized - "Detecting invalid
filters"

Nit: Opening bracket on next line please (same for the next test).

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:19: throws(this.parseFilter.bind(null,
"||example.com$unknown"), "unkown option");
Typo: "unkown"

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:27: text = "||example.com";
Nit: Better "||example.com^" - a separator placeholder should normally be used
at the end of a domain name.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:29: ok(filter instanceof this.BlockingFilter &&
filter.text == text, "blocking filter");
Here and everywhere else, please use a separate check for each requirement:

ok(filter instanceof this.BlockingFilter, "Blocking filter");
equal(filter.text, text, "Filter text");

This has two advantages:

* equal() will produce more useful output in case of failure - it will show you
the expected and actual value.
* You can immediately see which check failed.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:39: equal(this.parseFilter(""), null, "empty
filter");
Might be a good idea to test that filter normalization is applied, e.g. leading
and trailing spaces are being removed for all filter types. Note: testing filter
normalization itself here isn't necessary, that's core functionality and should
be tested there.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:77: /^([^:]+ )?4( [^:]+)?:/,
No point overcomplicating this, how about /\b4\b/? This has the advantage that
no assumptions about the string are made, merely the fact that the line number
is in there.

[Sebastian Noack, 2014-11-20 15:36:29]

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:13: this.CommentFilter =
filterClasses.CommentFilter;
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> It makes relatively little sense to require modules for each test run
> individually - these won't change from one test to another and don't need to
be
> cleaned up. You can do that at the top of the file instead.

Done, but I used an IFEE, to don't pollute the global context.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:18: test("detecting invalid filters", function()
{
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> Nit: Here and below, test names are usually capitalized - "Detecting invalid
> filters"
> 
> Nit: Opening bracket on next line please (same for the next test).

Done.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:19: throws(this.parseFilter.bind(null,
"||example.com$unknown"), "unkown option");
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> Typo: "unkown"

Done.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:27: text = "||example.com";
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> Nit: Better "||example.com^" - a separator placeholder should normally be used
> at the end of a domain name.

Done.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:29: ok(filter instanceof this.BlockingFilter &&
filter.text == text, "blocking filter");
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> Here and everywhere else, please use a separate check for each requirement:
> 
> ok(filter instanceof this.BlockingFilter, "Blocking filter");
> equal(filter.text, text, "Filter text");
> 
> This has two advantages:
> 
> * equal() will produce more useful output in case of failure - it will show
you
> the expected and actual value.
> * You can immediately see which check failed.

Done.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:39: equal(this.parseFilter(""), null, "empty
filter");
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> Might be a good idea to test that filter normalization is applied, e.g.
leading
> and trailing spaces are being removed for all filter types. Note: testing
filter
> normalization itself here isn't necessary, that's core functionality and
should
> be tested there.

Done.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:77: /^([^:]+ )?4( [^:]+)?:/,
On 2014/11/20 14:10:06, Wladimir Palant wrote:
> No point overcomplicating this, how about /\b4\b/? This has the advantage that
> no assumptions about the string are made, merely the fact that the line number
> is in there.

My concern was that there can be digits occurring in the actual error message,
but fair enough.

[Wladimir Palant, 2014-11-20 16:58:20]

Feel free to ignore the nit below, LGTM if you do.

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/5643440998055936/quni...
qunit/tests/filterValidation.js:13: this.CommentFilter =
filterClasses.CommentFilter;
On 2014/11/20 15:36:29, Sebastian Noack wrote:
> Done, but I used an IFEE, to don't pollute the global context.

Right, they don't run in their own context... Actually, the way this is solved
for Firefox is a separate `common.js` file that does all the requires.

http://codereview.adblockplus.org/5279235799252992/diff/4889511028850688/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/4889511028850688/quni...
qunit/tests/filterValidation.js:57: equal(filters.length, 3, "there are 3
filters");
Nit: You are duplicating the expected result in the message, something like "all
filters parsed" would be better.

[Sebastian Noack, 2014-11-21 08:08:08]

http://codereview.adblockplus.org/5279235799252992/diff/4889511028850688/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/5279235799252992/diff/4889511028850688/quni...
qunit/tests/filterValidation.js:57: equal(filters.length, 3, "there are 3
filters");
On 2014/11/20 16:58:20, Wladimir Palant wrote:
> Nit: You are duplicating the expected result in the message, something like
"all
> filters parsed" would be better.

Done, and improved a few other messages as well.

[Wladimir Palant, 2014-11-21 10:32:15]

LGTM