Issue 2376 - Implement custom filters in new options page

messageResponder.js

Index: messageResponder.js
===================================================================
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -212,6 +212,51 @@
         else
           FilterStorage.removeFilter(filter, subscription, message.index);
         break;
+      case "filters.importRaw":

[Thomas Greiner, 2015/06/30 09:23:27]

It's easier to search for the right method if the cases are ordered
alphabetically so could you please move them to the according position?

[saroyanm, 2015/07/08 18:25:39]

Done.

+        var parseFilters = require("filterValidation").parseFilters;
+        var result = parseFilters(message.text);
+
+        var errors = result.errors.filter(function(e)

[Thomas Greiner, 2015/06/30 09:23:27]

Please rename this variable from "e" to "error". Generally, I'd avoid shortening
variables wherever it makes sense to avoid confusion.

[saroyanm, 2015/07/08 18:25:39]

Done, Agree

+        {
+          return e.type != "unexpected-filter-list-header";

[Thomas Greiner, 2015/06/30 09:23:26]

I'm aware that you didn't write this but what's the reason for excluding this
kind of error?

[saroyanm, 2015/07/08 18:25:40]

It's being fired when the the filter starts with "[".
I can't see reason excluding this error. If it's invalid filter we should
somehow inform the user I guess. Should we remove the check ?

[Thomas Greiner, 2015/07/09 11:07:54]

Asked Sebastian about this and the line of reasoning is to not throw an error
when people copy&paste an entire filter list. Instead it silently strips the
filter list header.

So until we got a better solution for that let's keep it in there.

+        });
+
+        if (errors.length > 0)
+        {
+          alert(errors.join("\n"));
+          return;
+        }
+
+        var seenFilter = Object.create(null);
+        for (var i = 0; i < result.filters.length; i++)
+        {
+          var filter = result.filters[i];
+          FilterStorage.addFilter(filter);
+          seenFilter[filter.text] = null;
+        }
+
+        var remove = [];
+        for (var i = 0; i < FilterStorage.subscriptions.length; i++)
+        {
+          var subscription = FilterStorage.subscriptions[i];
+          if (!(subscription instanceof SpecialSubscription))
+            continue;
+
+          for (var j = 0; j < subscription.filters.length; j++)
+          {
+            var filter = subscription.filters[j];
+            if (filter instanceof WhitelistFilter && /^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))

[Thomas Greiner, 2015/06/30 09:23:27]

Beware of the 80 characters limit

[saroyanm, 2015/07/08 18:25:39]

Done.

+              continue;
+
+            if (!(filter.text in seenFilter))
+              FilterStorage.removeFilter(filter);
+          }
+        }
+        break;
+      case "parse.filter":

[Thomas Greiner, 2015/06/30 09:23:27]

This name is inconsistent with the other ones which follow the pattern
`<object>.<action>`. Therefore I'd suggest renaming it to "filters.parse".

[saroyanm, 2015/07/08 18:25:40]

Done.

+        var parseFilter = require("filterValidation").parseFilter;

[Thomas Greiner, 2015/06/30 09:23:26]

No need to require it each time it's called so please move this to the beginning
of the file where you already resolve all of the other dependencies.

Also found this in "filters.importRaw".

[saroyanm, 2015/07/08 18:25:40]

Agree, was stupid.

+        callback(parseFilter(message.text));

[Thomas Greiner, 2015/06/30 09:23:26]

"parseFilter" returns an instance of `Filter` (e.g. `WhitelistFilter`). However,
those object types don't exist in the context of the options page so you need to
convert it to a plain object before using `convertFilter`.
Same applies to `FilterParsingError` for which, however, there's no conversion
function yet.

var result = parseFilter(message.text);
callback({
  error: convertFilterParsingError(result.error),
  filter: convertFilter(result.filter)
});

[saroyanm, 2015/07/08 18:25:39]

Good point, done.

+        break;
       case "prefs.get":
         callback(Prefs[message.key]);
         break;