Issue 2390 - Created filter class for CSS property filters

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -20,6 +20,7 @@
  */
 
 let {FilterNotifier} = require("filterNotifier");
+let {Utils} = require("utils");
 
 /**
  * Abstract base class for filters
@@ -85,6 +86,11 @@
  * @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
@@ -160,6 +166,27 @@
 };
 
 /**
+ * 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
@@ -543,20 +570,7 @@
     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;
@@ -861,7 +875,7 @@
  * @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)
 {
@@ -883,10 +897,7 @@
         }
         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;
         }
@@ -898,15 +909,24 @@
     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);
+  }
 };
 
 /**
@@ -946,3 +966,64 @@
 {
   __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;
+  }
+};