Issue 2376 - Implement custom filters in new options page

background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 135 matching lines @@
     {
       this.text = text;
       this.disabled = false;
     }
   };
   modules.filterClasses.Filter.fromText = function(text)
   {
     return new modules.filterClasses.Filter(text);
   };
 
+  modules.filterValidation = 

[Sebastian Noack, 2015/07/14 13:36:31]

I wonder whezther this implementation might go a little to far for a mock. How
about simply making it noop?

modules.filterValidation = {
  parseFilter: modules.filterClasses.Filter.fromText,
  parseFilters: function(text)
  {
    return text.split("\n").map(modules.filterClasses.Filter.fromText);
  }
};

[saroyanm, 2015/07/14 13:53:28]

parseFilter: In this case we wouldn't be able to return FilterParsingError,
which helps testing locally for errors. 
parseFilters: this method should return {filters: filters, errors: errors},
again helps testing locally, at least returning similar format, that we have for
production code.

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

Sorry, I meant:

  modules.filterValidation = {
    parseFilter: function(text)
    {
      return {filter: modules.filterClasses.Filter.fromText(text)};
    },
    parseFilters: function(text)
    {
      return {filters:
text.split("\n").map(modules.filterClasses.Filter.fromText)};
    }
  };

Sure testing validation errors with the UI only wouldn't be possible then. But I
was wondering whether testing these (outside of the extension) is worth the
added complexity and duplicated logic. Thomas?

[Thomas Greiner, 2015/07/14 14:20:29]

That's true, for testing purposes we don't need to duplicate any parts of the
validation in the UI. It's still valuable to be able to test error handling in
the test environment so for that we could define some way that always throws a
generic filter validation error in parseFilter(s).

e.g. A URL parameter that causes the parseFilter(s) function to return an error
OR specify a character that - when included in a filter text - causes an error.

[Sebastian Noack, 2015/07/14 14:25:27]

+1 (this would also be in line with how we test the data corruption warning on
the first run page, and different languages)

[saroyanm, 2015/07/14 15:08:26]

Please note that error should be an instance of "FilterParsingError".
also code is not fully duplicated, there was more robust check for different
errors.
parseFilters doesn't return {filters: filters}, but {filters: fiters, errors:
errors}
I would say make sense to change parsing error to anything else while we are
excluding "unexpected-filter-list-header" in filters.importRaw, but it's making
the testing more nautral, just enter "[" in textbox or in textarea several times
to test it.
Updated the code please let me know if you are Okey with current one, otherwise
I can go with your suggestion.

[Sebastian Noack, 2015/07/14 16:48:38]

Actually, we don't need to implement the FilterParsingError class here. We could
simply use a string or window.Error object instead. It's toString() method will
return the message, and its "type" attribute will be undefined an therefore
won't equal "unexpected-filter-list-header".
This behavior isn't obvious. There are several error scenarios (in the
extension), and you have to look at the code to know which one is implemented
here, to test the validation error scenario outside of the extension. Moreover,
the way you implemented it is inconsistent. In the extension, lines starting
with "[" will be stripped when importing raw filters. With your implementation
it will cause an error.

So we either end up duplicating an unreasonable amount of the logic we have in
the extension here, and the current implementation already goes a little to far
for my taste. Or we return an error here, regardless of the input, but
depdendant on an URL parameter, as greiner suggested, similar to
"seenDataCorruption" and a couple of others we already have for similar purposes
(see the README).
You merely slightly simplified the current logic. That is not what we were
talking about.

[saroyanm, 2015/07/14 18:28:27]

Done.

+  {
+    FilterParsingError: function(type)
+    {
+      this.type = type;
+    },
+    parseFilter: function(text) 
+    {
+      if (text)
+      {
+        if (text[0] == "[")
+          return {error: new modules.filterValidation.FilterParsingError("unexpected-filter-list-header")};
+      }
+      return {filter: modules.filterClasses.Filter.fromText(text)};
+    },
+    parseFilters: function(text)
+    {
+      var lines = text.split("\n");
+      var filters = [];
+      var errors = [];
+
+      for (var i = 0; i < lines.length; i++)
+      {
+        var parseResult = modules.filterValidation.parseFilter(lines[i]);
+        if (parseResult.error)
+          errors.push(parseResult.error);
+        else
+          filters.push(parseResult.filter);
+      }
+
+      return {filters: filters, errors: errors};
+    }
+  };
+  modules.filterValidation.FilterParsingError.prototype.toString = function()
+  {
+    return this.type;
+  };
+
   modules.synchronizer = {
     Synchronizer: {}
   };
 
   modules.matcher = {
     defaultMatcher: {
       matchesAny: function(url, requestType, docDomain, thirdParty)
       {
         var params = {blockedURLs: ""};
         updateFromURL(params);
@@ skipping 48 matching lines @@
       {
         return parseFloat(v1) - parseFloat(v2);
       }
     }
   };
 
   var filters = [
     "@@||alternate.de^$document",
     "@@||der.postillion.com^$document", 
     "@@||taz.de^$document",
-    "@@||amazon.de^$document"
+    "@@||amazon.de^$document",
+    "||biglemon.am/bg_poster/banner.jpg",
+    "winfuture.de###header_logo_link",
+    "###WerbungObenRechts10_GesamtDIV",
+    "###WerbungObenRechts8_GesamtDIV",
+    "###WerbungObenRechts9_GesamtDIV",
+    "###WerbungUntenLinks4_GesamtDIV",
+    "###WerbungUntenLinks7_GesamtDIV",
+    "###WerbungUntenLinks8_GesamtDIV",
+    "###WerbungUntenLinks9_GesamtDIV",
+    "###Werbung_Sky",
+    "###Werbung_Wide",
+    "###__ligatus_placeholder__",
+    "###ad-bereich1-08",
+    "###ad-bereich1-superbanner",
+    "###ad-bereich2-08",
+    "###ad-bereich2-skyscrapper"
   ];
   var knownFilters = filters.map(modules.filterClasses.Filter.fromText);
 
   var subscriptions = [
     "https://easylist-downloads.adblockplus.org/easylistgermany+easylist.txt",
     "https://easylist-downloads.adblockplus.org/exceptionrules.txt",
     "https://easylist-downloads.adblockplus.org/fanboy-social.txt",
     "~user~786254"
   ];
   var knownSubscriptions = Object.create(null);
@@ skipping 18 matching lines @@
         type: "message",
         payload: {
           title: "Custom subscription",
           url: "http://example.com/custom.txt",
           type: "add-subscription"
         }
       }, "*");
     }, 1000);
   }
 })(this);