Issue 491 - Validate custom filters

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -22,6 +22,7 @@
 {
   this.Filter = Filter;
   this.WhitelistFilter = WhitelistFilter;
+  this.InvalidFilter = InvalidFilter;
 }
 with(require("subscriptionClasses"))
 {
@@ -473,11 +474,19 @@
   event.preventDefault();
 
   var filterText = Filter.normalize(document.getElementById("newFilter").value);
+  if (filterText)
+  {

[kzar, 2014/11/18 16:32:10]

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?

[Sebastian Noack, 2014/11/18 17:42:55]

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/.

+    var filter = Filter.fromText(filterText, true)
+    if (filter instanceof InvalidFilter)
+    {
+      alert(filter.reason);
+      return;

[kzar, 2014/11/18 16:32:10]

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.

[Sebastian Noack, 2014/11/18 17:42:55]

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

+    }
+
+    FilterStorage.addFilter(filter);
+  }
+
   document.getElementById("newFilter").value = "";
-  if (!filterText)
-    return;
-
-  FilterStorage.addFilter(Filter.fromText(filterText));
 }
 
 // Removes currently selected whitelisted domains
@@ -529,9 +538,10 @@
 // Imports filters in the raw text box
 function importRawFiltersText()
 {
-  $("#rawFilters").hide();
   var filters = document.getElementById("rawFiltersText").value.split("\n");
   var seenFilter = {__proto__: null};
+
+  var add = [];
   for (var i = 0; i < filters.length; i++)
   {
     var text = Filter.normalize(filters[i]);
@@ -542,9 +552,18 @@
     if (/^\[/.test(text))
       continue;

[Wladimir Palant, 2014/11/19 16:03:11]

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]

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.

 
-    FilterStorage.addFilter(Filter.fromText(text));
+    var filter = Filter.fromText(text, true);

[kzar, 2014/11/18 16:32:10]

Again seems like this logic is duplicate of above and logic in background.js

+    if (filter instanceof InvalidFilter)
+    {
+      alert(filter.reason);
+      return;
+    }

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

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);
  }

+
+    add.push(filter);
     seenFilter[text] = true;
   }
+  for (var i = 0; i < add.length; i++)
+    FilterStorage.addFilter(add[i]);
 
   var remove = [];
   for (var i = 0; i < FilterStorage.subscriptions.length; i++)
@@ -565,6 +584,8 @@
   }
   for (var i = 0; i < remove.length; i++)
     FilterStorage.removeFilter(remove[i]);
+
+  $("#rawFilters").hide();
 }
 
 // Called when user explicitly requests filter list updates