Issue 2376 - Implement custom filters in new options page

messageResponder.js

Index: messageResponder.js
===================================================================
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -28,8 +28,11 @@
   
   var filterClasses = require("filterClasses");
   var Filter = filterClasses.Filter;
+  var WhitelistFilter = filterClasses.WhitelistFilter
   var BlockingFilter = filterClasses.BlockingFilter;
   var Synchronizer = require("synchronizer").Synchronizer;
+  var parseFilters = require("filterValidation").parseFilters;
+  var parseFilter = require("filterValidation").parseFilter;
 
   var subscriptionClasses = require("subscriptionClasses");
   var Subscription = subscriptionClasses.Subscription;
@@ -46,6 +49,7 @@
 
   var convertSubscription = convertObject.bind(null, ["disabled",
     "downloadStatus", "homepage", "lastSuccess", "title", "url"]);
+  var convertFilterParsingError = convertObject.bind(null, ["type"]);
   var convertFilter = convertObject.bind(null, ["text"]);
 
   var changeListeners = null;
@@ -195,12 +199,61 @@
         
         callback(subscription.filters.map(convertFilter));
         break;
+      case "filters.importRaw":
+        var result = parseFilters(message.text);
+        var errors = result.errors.filter(function(error)
+        {
+          return error.type != "unexpected-filter-list-header";
+        });
+
+        if (errors.length > 0)
+        {
+          alert(errors.join("\n"));

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

This won't work because this code is running in the background page. Just like
when adding individual filters we need two separate messages: one for parsing
the filters and one for adding them.

[Sebastian Noack, 2015/07/10 07:31:00]

What could go wrong when adding filters? Currently, only parsing errors are
handled. But yeah, alert() from within the background page is inappropriate as
explained in my other comment.

[Thomas Greiner, 2015/07/10 12:38:52]

We're talking about the same issue: the placement of the alert().

[saroyanm, 2015/07/13 14:05:34]

Done.

+          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 &&

[Sebastian Noack, 2015/07/09 12:12:14]

Nit: Isn't |filter instanceof WhilelistFilter| redundant, as the regexp already
excludes other kind of filters, by requiring the filter text to start with "@@"?

[saroyanm, 2015/07/09 16:31:41]

Yes this redundant, but it's the way it was implemented in old version of option
page, I thought maybe there is a reason of doing so, similar as you can see my
answer below.

[Sebastian Noack, 2015/07/10 07:31:00]

Just because the previous code is doing something, isn't reason enough to keep
doing it like that if there is a better option. And I cannot think of any reason
why the redundant instanceof check here is necessary.

[saroyanm, 2015/07/13 14:05:34]

Done.

+                /^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))
+              continue;
+
+            if (!(filter.text in seenFilter))
+              remove.push(filter);
+          }
+        }
+        for (var i = 0; i < remove.length; i++)
+          FilterStorage.removeFilter(remove[i]);

[Sebastian Noack, 2015/07/09 12:12:14]

Any reason to collect the filters to be removed in an array, removing them in a
seperate loop, instead simply calling FilterStorage.removeFilter() above?

[saroyanm, 2015/07/09 16:31:40]

In that case we will have object relation issue, because we will modify array
each time we are removing item, we faced this issue in previous. Thomas just
found the issue with the solution. Currently I'm slicing the array.

[Sebastian Noack, 2015/07/10 07:31:00]

What one would usually do in this case, is simply iterating over the array in
reverse order, starting at the last item towards the first:

  for (var i = FilterStorage.subscriptions.length - 1; i >= 0; i--)
  {
    var subscription = FilterStorage.subscriptions[i];

    for (var j = subscription.filters.length - 1; j >= 0; j--)
    {
      var filter = subscription.filters[j];

      ...

      FilterStorage.removeFilter(filter);
    }
  }

That way, if an item gets removed, it doesn't change the index of the previous
items to be processed next. This will be simpler and faster than creating a
separate array.

[Thomas Greiner, 2015/07/10 12:38:53]

However, it's just as confusing as the current method. Therefore I'd still
recommend creating a copy of the array because, if possible, you shouldn't
remove elements from the array you're iterating over.

Performance optimizations at this point are probably premature optimization
unless it turns out that the lag is noticeably bigger.

[Sebastian Noack, 2015/07/14 11:57:24]

If you keep the performance aspect aside, I suppose it's just personal
preference. I'd certainly prefer to iterate over an array from behind rather
than copying it. Another option would be to simply decrease the counter when you
removed an item.
Ever tried to save several thousand of filters in raw mode?

[Thomas Greiner, 2015/07/14 12:47:16]

Remembering what you said that some people copy entire filter lists into this
field, it may indeed be necessary to optimize for performance here, agreed.

[saroyanm, 2015/07/14 13:15:46]

Agree as well, done.

+        break;
       case "filters.listen":
         if (message.filter)
           listenerFilters.filter = message.filter;
         else
           delete listenerFilters.filter;
         break;
+      case "filter.parse":

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

Plural for consistency: "filters.parse"

[saroyanm, 2015/07/09 16:31:40]

Done.

+        var result = parseFilter(message.text);
+        if (result.filter)

[Sebastian Noack, 2015/07/09 12:12:14]

Note that if the normalized filter text is empty result.filter will be null, but
there won't be an error. So the logic here should probably look like that:

if (result.error)
  callback({error: convertFilterParsingError(result.error)});
else
  callback({filter: result.filter && convertFilter(result.filter)});

[saroyanm, 2015/07/09 16:31:40]

We changed the implementation here, we will need to implement error.listener and
for that I'm not sending the error msd through callback method.
I'll suggest to implement error handling in separate ticket.

[Sebastian Noack, 2015/07/10 07:31:00]

This sounds rather complicated and backwards to me. Simply returning the
corresponding error with the API that it occurred with, sounds more appropriate
and simpler to implement to me.

[Thomas Greiner, 2015/07/10 12:38:53]

It would be more inline with how we return filters that were added using
"filters.add" and note that we'll need the ability to listen to errors later on
anyway.

[Sebastian Noack, 2015/07/14 11:57:24]

Yes, we need to listen for subscription download errors as well. But these are
unrelated and shown in a different way. So I don't see much a point in reporting
all kind of errors with the same message. I think it makes much more sense and
would be simpler to return validation errors, in the response to the (only)
message that can cause them.

[saroyanm, 2015/07/14 13:15:46]

Currently we have implemented global error reporting, in case I'll change it to
the callback I'll need to remove current implementation, because we won't use it
in current code, but we are going to use later even if we will implement
callback error handling, so I'll suggest to have this error handling for now and
implement the callback error handling for specific element afterward when we
will have request for that.

+          callback({filter: convertFilter(result.filter)});
+        else
+          callback({error: convertFilterParsingError(result.error)});
+        break;
       case "filters.remove":
         var filter = Filter.fromText(message.text);
         var subscription = null;