Adding ABP core modules to ABP/Opera

lib/matcher.js

 /*
  * This Source Code is subject to the terms of the Mozilla Public License
  * version 2.0 (the "License"). You can obtain a copy of the License at
  * http://mozilla.org/MPL/2.0/.
  */
 
 //
 // This mimicks the API of the Matcher module in ABP/Firefox but users Opera's
 // URLFilter API.
 //
 
 (function()
 {
   var WhitelistFilter = null;
   var RegExpFilter = null;
   var resourceTypes = [
       "DOCUMENT", "FONT", "IMAGE", "MEDIA", "OBJECT", "OBJECT_SUBREQUEST",
       "OTHER", "SCRIPT", "STYLESHEET", "SUBDOCUMENT", "XMLHTTPREQUEST"
   ];
 
   require.scopes.matcher =
   {
     defaultMatcher:
     {
       _rules: {},
 
-      _generateRule: function(filter)

[Felix Dahlke, 2012/10/10 12:11:22]

I'd love it if the code segments below comments ("Convert anchors, ...",
"Translate type options" etc.) would be extracted to functions. Probably just a
matter of taste, but I find small functions with descriptive names easier to
understand than larger ones with comments. I've also given Andrey that feedback
a lot - it's just my opinion, nothing I insist on.

+      /**
+       * Converts rule text from Adblock Plus format (implicit * at beginning
+       * and end of the rule unless there is an anchor) to Opera format (* has
+       * to be specified explicitly).
+       */
+      _getRuleText: function(/**Filter*/ filter) /**String*/
       {
-        if (!WhitelistFilter)
-        {
-          WhitelistFilter = require("filterClasses").WhitelistFilter;
-          RegExpFilter = require("filterClasses").RegExpFilter;
-        }
-
-        var rule = {
-          type: filter instanceof WhitelistFilter ? "allow" : "block",
-          text: filter.regexpSource,
-          options: {}
-        };
-
-        // Convert anchors, Opera requires explicit *
+        var text = filter.regexpSource;
 
         // |foo => foo
         // foo => *foo
         // *foo and ||foo stay unchanged
-        if (rule.text.substr(0, 2) != "||")
+        if (text.substr(0, 2) != "||")
         {
-          if (rule.text.substr(0, 1) == "|")

[Felix Dahlke, 2012/10/10 12:11:22]

This value is used twice, so how about storing it in a variable? I'd also prefer
[0] to .substr(0, 1):

var firstChar = rule.text[0];
if (firstChar == "|")
...

Only a readability aid, we can of course rely on the idempotency of [] and
substr().

-            rule.text = rule.text.slice(1);
-          else if (rule.text.substr(0, 1) != "*")
-            rule.text = "*" + rule.text;
+          var start = text.substr(0, 1);
+          if (start == "|")
+            text = text.slice(1);
+          else if (start != "*")
+            text = "*" + text;
         }
 
         // foo| => foo
         // foo => foo*
         // foo* stays unchanged
-        if (rule.text.substr(-1) == "|")

[Felix Dahlke, 2012/10/10 12:11:22]

As above, how about storing this in a variable lastChar?

-          rule.text = rule.text.slice(0, -1);
-        else if (rule.text.substr(-1) != "*")
-          rule.text = rule.text + "*";
+        var end = text.substr(-1);
+        if (end == "|")
+          text = text.slice(0, -1);
+        else if (end != "*")
+          text = text + "*";
 
-        // Translate type options
-        var resources = 0;
+        return text;
+      },
+
+      /**
+       * Converts type options of a filter into a bit mask as expected by Opera.
+       */
+      _getRuleTypes: function(/**Filter*/ filter) /**Integer*/
+      {
+        var types = 0;
         var urlfilter = opera.extension.urlfilter;
         for (var i = 0; i < resourceTypes.length; i++)
         {
           var type = resourceTypes[i];
           if (filter.contentType & RegExpFilter.typeMap[type])
-            resources = resources | urlfilter["RESOURCE_" + type];

[Felix Dahlke, 2012/10/10 12:11:22]

How about:
resources |= urlfilter["RESOURCE_" + type];

[Wladimir Palant, 2012/10/11 09:36:26]

I thought that JavaScript doesn't support |= but it seems that I was mistaken -
looks like I was thinking about ||= that is indeed not supported.

+            types |= urlfilter["RESOURCE_" + type];
         }
-        rule.options.resources = resources;
+        return types;
+      },
 
-        // Translate domain options
+      /**
+       * Converts domain options of a filter into includeDomains/excludeDomains
+       * options as expected by Opera. The options object has to be passed in,
+       * properties will be added to it if necessary.
+       */
+      _getRuleDomains: function(/**Filter*/ filter, /**Object*/ options)
+      {
         if (filter.domainSource)
         {
           var domains = filter.domainSource.split(filter.domainSeparator);
           for (var i = 0; i < domains.length; i++)
           {
             var domain = domains[i];
             if (domain == "")
               continue;
 
             var type = "includeDomains";
             if (domain[0] == "~")
             {
               type = "excludeDomains";
               domain = domain.substr(1);
             }
-            if (!(type in rule.options))
-              rule.options[type] = [];
-            rule.options[type].push(domain);
+            if (!(type in options))
+              options[type] = [];
+            options[type].push(domain);
           }
         }
+      },
 
-        // Translate third-party option
+      /**
+       * Converts third-party filter option to the format expected by Opera.
+       * The options object has to be passed in, a property will be added to it
+       * if necessary.
+       */
+      _getRuleThirdParty: function(/**Filter*/ filter, /**Object*/ options)
+      {
         if (filter.thirdParty !== null)
-          rule.options.thirdParty = filter.thirdParty;
+          options.thirdParty = filter.thirdParty;
+      },
 
+      /**
+       * Converts an Adblock Plus filter to a rule that can be processed by
+       * Opera. The following options will be set:
+       *
+       *  type: urlfilter to be used, either "allow" or "block"
+       *  text: rule text
+       *  options: rule options
+       */
+      _generateRule: function(/**Filter*/ filter) /**Object*/
+      {
+        if (!WhitelistFilter)
+        {
+          WhitelistFilter = require("filterClasses").WhitelistFilter;
+          RegExpFilter = require("filterClasses").RegExpFilter;
+        }
+
+        var rule = {
+          type: filter instanceof WhitelistFilter ? "allow" : "block",
+          text: this._getRuleText(filter),
+          options: {
+            resources: this._getRuleTypes(filter)
+          }
+        };
+        this._getRuleDomains(filter, rule.options);
+        this._getRuleThirdParty(filter, rule.options);
         return rule;
       },
 
       add: function(filter)
       {
         if (filter.text in this._rules)
           return;
 
         var rule = this._generateRule(filter);
-        if (filter.text == "http://")

[Felix Dahlke, 2012/10/10 12:11:22]

Why just check for the string "http://" here? Wouldn't "foobar" or anything
that's not a valid filter be equally worth reporting?

[Wladimir Palant, 2012/10/11 09:36:26]

This is just debugging code I forgot :)

-          Cu.reportError(JSON.stringify(rule));
         opera.extension.urlfilter[rule.type].add(rule.text, rule.options);
         this._rules[filter.text] = rule;
       },
 
       remove: function(filter)
       {
         if (!(filter.text in this._rules))
           return;
 
         var rule = this._rules[filter.text];
         opera.extension.urlfilter[rule.type].remove(rule.text);
         delete this._rules[filter.text];
       },
 
-      clear: function(filter)
+      clear: function()
       {
         for (var text in this._rules)
         {
           var rule = this._rules[text];
-          opera.extension.urlfilter[rule.type].remove(rule);

[Felix Dahlke, 2012/10/10 12:11:22]

Shouldn't this be "remove(text)"?

+          opera.extension.urlfilter[rule.type].remove(rule.text);
         }
         this._rules = {};
       }
     }
   };
 })();