Issue 2390 - Created filter class for CSS property filters

lib/filterClasses.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * @fileOverview Definition of Filter class and its subclasses.
  */
 
 let {FilterNotifier} = require("filterNotifier");
+let {Utils} = require("utils");
 
 /**
  * Abstract base class for filters
  *
  * @param {String} text   string representation of the filter
  * @constructor
  */
 function Filter(text)
 {
   this.text = text;
@@ skipping 45 matching lines @@
 /**
  * Regular expression that RegExp filters specified as RegExps should match
  * @type RegExp
  */
 Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w\-]+(?:=[^,\s]+)?(?:,~?[\w\-]+(?:=[^,\s]+)?)*)?$/;
 /**
  * Regular expression that options on a RegExp filter should match
  * @type RegExp
  */
 Filter.optionsRegExp = /\$(~?[\w\-]+(?:=[^,\s]+)?(?:,~?[\w\-]+(?:=[^,\s]+)?)*)$/;
+/**
+ * Regular expression that CSS property filters should match
+ * @type RegExp
+ */
+Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/;

[Sebastian Noack, 2015/05/05 15:47:02]

So we don't allow quotes in the value here. Should be fine for now, as I  can
only think of a few esoteric CSS properties that use quotes. But please mention
it in the comment above.

[Thomas Greiner, 2015/05/06 12:08:32]

Done, I added a comment as suggested. We could change this as soon as we need it
without breaking backwards compatibility because for now it would make the regex
unnecessarily complicated.

[Thomas Greiner, 2015/05/06 15:33:07]

Yes, there's currently no negative lookbehind in JavaScript which would allow us
to recognize `\"` as an escaped quotation mark.

Just FYI: Using /\[\-abp\-properties=(["'])((?:(?!\1).)+)\1\]/ we could at least
allow the other character that's not used for limiting the string. The only
strings that aren't allowed in this scenario are strings which contain both
quotation characters (e.g. `[-abp-properties="content: \"Don't do this\";"]`).
The issue with it is performance, however, since it requires a negative
lookahead (see http://jsperf.com/allowing-quotation-marks-in-strings).

[Sebastian Noack, 2015/05/06 15:42:29]

Yeah, let's stick to not allowing any quotes for now then, though it only seems
to make a notable difference on modern Firefox versions.

 
 /**
  * Creates a filter of correct type from its text representation - does the basic parsing and
  * calls the right constructor then.
  *
  * @param {String} text   as in Filter()
  * @return {Filter}
  */
 Filter.fromText = function(text)
 {
@@ skipping 55 matching lines @@
   {
     // Special treatment for element hiding filters, right side is allowed to contain spaces
     let [, domain, separator, selector] = /^(.*?)(#\@?#?)(.*)$/.exec(text);
     return domain.replace(/\s/g, "") + separator + selector.trim();
   }
   else
     return text.replace(/\s/g, "");
 };
 
 /**
+ * Converts filter text into regular expression string
+ * @param {String} text as in Filter()
+ * @return {String} regular expression representation of filter text
+ */
+Filter.toRegExp = function(text)
+{
+  return text
+    .replace(/\*+/g, "*")        // remove multiple wildcards
+    .replace(/\^\|$/, "^")       // remove anchors following separator placeholder
+    .replace(/\W/g, "\\$&")      // escape special symbols
+    .replace(/\\\*/g, ".*")      // replace wildcards by .*
+    // process separator placeholders (all ANSI characters but alphanumeric characters and _%.-)
+    .replace(/\\\^/g, "(?:[\\x00-\\x24\\x26-\\x2C\\x2F\\x3A-\\x40\\x5B-\\x5E\\x60\\x7B-\\x7F]|$)")
+    .replace(/^\\\|\\\|/, "^[\\w\\-]+:\\/+(?!\\/)(?:[^\\/]+\\.)?") // process extended anchor at expression start
+    .replace(/^\\\|/, "^")       // process anchor at expression start
+    .replace(/\\\|$/, "$")       // process anchor at expression end
+    .replace(/^(\.\*)/, "")      // remove leading wildcards
+    .replace(/(\.\*)$/, "");     // remove trailing wildcards
+}
+
+/**
  * Class for invalid filters
  * @param {String} text see Filter()
  * @param {String} reason Reason why this filter is invalid
  * @constructor
  * @augments Filter
  */
 function InvalidFilter(text, reason)
 {
   Filter.call(this, text);
 
@@ skipping 363 matching lines @@
    * @type RegExp
    */
   get regexp()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
     let prop = Object.getOwnPropertyDescriptor(this, "regexp");
     if (prop)
       return prop.value;
 
-    // Remove multiple wildcards
-    let source = this.regexpSource
-      .replace(/\*+/g, "*")        // remove multiple wildcards
-      .replace(/\^\|$/, "^")       // remove anchors following separator placeholder
-      .replace(/\W/g, "\\$&")      // escape special symbols
-      .replace(/\\\*/g, ".*")      // replace wildcards by .*
-      // process separator placeholders (all ANSI characters but alphanumeric characters and _%.-)
-      .replace(/\\\^/g, "(?:[\\x00-\\x24\\x26-\\x2C\\x2F\\x3A-\\x40\\x5B-\\x5E\\x60\\x7B-\\x7F]|$)")
-      .replace(/^\\\|\\\|/, "^[\\w\\-]+:\\/+(?!\\/)(?:[^\\/]+\\.)?") // process extended anchor at expression start
-      .replace(/^\\\|/, "^")       // process anchor at expression start
-      .replace(/\\\|$/, "$")       // process anchor at expression end
-      .replace(/^(\.\*)/, "")      // remove leading wildcards
-      .replace(/(\.\*)$/, "");     // remove trailing wildcards
-
+    let source = Filter.toRegExp(this.regexpSource);
     let regexp = new RegExp(source, this.matchCase ? "" : "i");
     Object.defineProperty(this, "regexp", {value: regexp});
     return regexp;
   },
   /**
    * Content types the filter applies to, combination of values from RegExpFilter.typeMap
    * @type Number
    */
   contentType: 0x7FFFFFFF,
   /**
@@ skipping 284 matching lines @@
 };
 
 /**
  * Creates an element hiding filter from a pre-parsed text representation
  *
  * @param {String} text       same as in Filter()
  * @param {String} domain     domain part of the text representation (can be empty)
  * @param {String} tagName    tag name part (can be empty)
  * @param {String} attrRules  attribute matching rules (can be empty)
  * @param {String} selector   raw CSS selector (can be empty)
- * @return {ElemHideFilter|ElemHideException|InvalidFilter}
+ * @return {ElemHideFilter|ElemHideException|CSSPropertyFilter|InvalidFilter}
  */
 ElemHideBase.fromText = function(text, domain, isException, tagName, attrRules, selector)
 {
   if (!selector)
   {
     if (tagName == "*")
       tagName = "";
 
     let id = null;
     let additional = "";
     if (attrRules) {
       attrRules = attrRules.match(/\([\w\-]+(?:[$^*]?=[^\(\)"]*)?\)/g);
       for (let rule of attrRules) {
         rule = rule.substr(1, rule.length - 2);
         let separatorPos = rule.indexOf("=");
         if (separatorPos > 0) {
           rule = rule.replace(/=/, '="') + '"';
           additional += "[" + rule + "]";
         }
         else {
           if (id)
-          {
-            let {Utils} = require("utils");
             return new InvalidFilter(text, Utils.getString("filter_elemhide_duplicate_id"));
-          }
           else

[Sebastian Noack, 2015/05/05 15:47:02]

Nit: Redundant else statement.

[Thomas Greiner, 2015/05/06 12:08:32]

Done.

             id = rule;
         }
       }
     }
 
     if (id)
       selector = tagName + "." + id + additional + "," + tagName + "#" + id + additional;
     else if (tagName || additional)
       selector = tagName + additional;
     else
-    {
-      let {Utils} = require("utils");
       return new InvalidFilter(text, Utils.getString("filter_elemhide_nocriteria"));
-    }
   }
   if (isException)
     return new ElemHideException(text, domain, selector);
   else

[Sebastian Noack, 2015/05/05 15:47:02]

Nit: Redundant else statement.

[Thomas Greiner, 2015/05/06 12:08:32]

Done.

-    return new ElemHideFilter(text, domain, selector);
+  {
+    if (Filter.csspropertyRegExp.test(selector))
+    {
+      // CSS property filters are inefficient so we need to make sure that
+      // they're only applied if they specify active domains
+      if (/,[^~]/.test("," + domain))

[Sebastian Noack, 2015/05/05 15:47:02]

I wonder whether we should also look for at least one dot. If we want to require
CSS poeperty filters to be limitted to a domain, allowing top level domains
would be kinda silly IMO.

[Thomas Greiner, 2015/05/06 12:08:32]

Good point but note that this still doesn't cover common two-part TLDs such as
"co.uk"?

[Sebastian Noack, 2015/05/06 13:07:48]

Hmm, if we want to do it properly, I suppose we have to use basedomain
functionality. It's built into Gecko, and we already have an own implementation
of it in the platform code for isThirdParty(). But it's probably not necessary
here.

+        return new CSSPropertyFilter(text, domain, selector);
+      else

[Sebastian Noack, 2015/05/05 15:47:02]

Nit: Redundant else statement.

+        return new InvalidFilter(text, Utils.getString("filter_cssproperty_nodomain"));
+    }
+    else

[Sebastian Noack, 2015/05/05 15:47:02]

Nit: Redundant else statement.

[Thomas Greiner, 2015/05/06 12:08:32]

Done.

+      return new ElemHideFilter(text, domain, selector);
+  }
 };
 
 /**
  * Class for element hiding filters
  * @param {String} text see Filter()
  * @param {String} domains  see ElemHideBase()
  * @param {String} selector see ElemHideBase()
  * @constructor
  * @augments ElemHideBase
  */
@@ skipping 19 matching lines @@
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideException = ElemHideException;
 
 ElemHideException.prototype =
 {
   __proto__: ElemHideBase.prototype
 };
+
+/**
+ * Class for CSS property filters
+ * @param {String} text       see Filter()
+ * @param {String} domains    see ElemHideBase()
+ * @param {String} selector   see ElemHideBase()
+ * @constructor
+ * @augments ElemHideBase
+ */
+function CSSPropertyFilter(text, domains, selector)
+{
+  ElemHideBase.call(this, text, domains, selector);
+
+  let properties;
+  [properties, , this.regexpSource] = selector.match(Filter.csspropertyRegExp);
+  [this.selectorPrefix, this.selectorSuffix] = selector.split(properties);
+}
+exports.CSSPropertyFilter = CSSPropertyFilter;
+
+CSSPropertyFilter.prototype =
+{
+  __proto__: ElemHideBase.prototype,
+
+  /**
+   * Expression from which a regular expression should be generated for matching
+   * CSS properties - for delayed creation of the regexp property
+   * @type String
+   */
+  regexpSource: null,
+  /**
+   * Substring of CSS selector before properties for the HTML elements that
+   * should be hidden
+   * @type String
+   */
+  selectorPrefix: null,
+  /**
+   * Substring of CSS selector after properties for the HTML elements that
+   * should be hidden
+   * @type String
+   */
+  selectorSuffix: null,
+
+  /**
+   * Regular expression to be used when testing CSS properties against
+   * this filter
+   * @type RegExp
+   */
+  get regexp()

[Sebastian Noack, 2015/05/05 15:47:02]

Hmm, this is almost the same code we already have in RegExpFilter.regexp. How
about moving the whole logic to ActiveFiler._getRegExp()?

However, on second thought, creating a RegExp object here, seems to be even
unneeded, as we have to match the regexp in the content script. So I think we
actually just need a regexp string here.

[Thomas Greiner, 2015/05/06 12:08:32]

You're right, the issue description doesn't explicitly say that `regexp()`
should be of type `RegExp`. It might be a bit unexpected though, that it returns
a string so I renamed it.

However, there might be reasons for converting it right away so I'd like to hear
Wladimir's opinion on that as well since he created the issue.

[Sebastian Noack, 2015/05/06 13:07:48]

Well, objects get serialized when passed to content scripts. So dependent on the
browser it either won't be able to serialize complex objects like regular
expressions, or will just convert it back into a string and reconstruct it
during deserialization.

+  {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "regexp");
+    if (prop)
+      return prop.value;
+
+    let source = Filter.toRegExp(this.regexpSource);
+    let regexp = new RegExp(source, "");
+    Object.defineProperty(this, "regexp", {value: regexp});
+    return regexp;
+  }
+};