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
@@ skipping 70 matching lines @@
  * @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
+ * Properties must not contain " or '
  * @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 607 matching lines @@
         collapse = true;
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && typeof value != "undefined")
         sitekeys = value;
       else
         return new InvalidFilter(origText, "Unknown option " + option.toLowerCase());
     }
   }
 
-  if (!blocking && (contentType == null || (contentType & RegExpFilter.typeMap.DOCUMENT)) &&
-      (!options || options.indexOf("DOCUMENT") < 0) && !/^\|?[\w\-]+:/.test(text))
-  {
-    // Exception filters shouldn't apply to pages by default unless they start with a protocol name
-    if (contentType == null)
-      contentType = RegExpFilter.prototype.contentType;
-    contentType &= ~RegExpFilter.typeMap.DOCUMENT;
-  }
-
   try
   {
     if (blocking)
       return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys, collapse);
     else
       return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, e);
@@ skipping 18 matching lines @@
   DTD: 1,
   MEDIA: 16384,
   FONT: 32768,
 
   BACKGROUND: 4,    // Backwards compat, same as IMAGE
 
   POPUP: 0x10000000,
   ELEMHIDE: 0x40000000
 };
 
-// ELEMHIDE, POPUP option shouldn't be there by default
-RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP);
+// DOCUMENT, ELEMHIDE, POPUP options shouldn't be there by default
+RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.DOCUMENT | RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP);
 
 /**
  * Class for blocking filters
  * @param {String} text see Filter()
  * @param {String} regexpSource see RegExpFilter()
  * @param {Number} contentType see RegExpFilter()
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
  * @param {String} sitekeys see RegExpFilter()
@@ skipping 99 matching lines @@
  */
 ElemHideBase.fromText = function(text, domain, isException, tagName, attrRules, selector)
 {
   if (!selector)
   {
     if (tagName == "*")
       tagName = "";
 
     let id = null;
     let additional = "";
-    if (attrRules) {
+    if (attrRules)
+    {
       attrRules = attrRules.match(/\([\w\-]+(?:[$^*]?=[^\(\)"]*)?\)/g);
-      for (let rule of attrRules) {
+      for (let rule of attrRules)
+      {
         rule = rule.substr(1, rule.length - 2);
         let separatorPos = rule.indexOf("=");
-        if (separatorPos > 0) {
+        if (separatorPos > 0)
+        {
           rule = rule.replace(/=/, '="') + '"';
           additional += "[" + rule + "]";
         }
-        else {
+        else
+        {
           if (id)
             return new InvalidFilter(text, Utils.getString("filter_elemhide_duplicate_id"));
-          else
-            id = rule;
+
+          id = rule;
         }
       }
     }
 
     if (id)
       selector = tagName + "." + id + additional + "," + tagName + "#" + id + additional;
     else if (tagName || additional)
       selector = tagName + additional;
     else
       return new InvalidFilter(text, Utils.getString("filter_elemhide_nocriteria"));
   }
+
   if (isException)
     return new ElemHideException(text, domain, selector);
-  else
-  {
-    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);
-  }
+
+  let match = Filter.csspropertyRegExp.exec(selector);
+  if (match)
+  {
+    // CSS property filters are inefficient so we need to make sure that
+    // they're only applied if they specify active domains
+    if (!/,[^~][^,.]*\.[^,]/.test("," + domain))
+      return new InvalidFilter(text, Utils.getString("filter_cssproperty_nodomain"));
+
+    return new CSSPropertyFilter(text, domain, selector, match[2],
+      selector.substr(0, match.index),
+      selector.substr(match.index + match[0].length));
+  }
+
+  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 22 matching lines @@
 }
 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()
+ * @param {String} text           see Filter()
+ * @param {String} domains        see ElemHideBase()
+ * @param {String} selector       see ElemHideBase()
+ * @param {String} regexpSource   see CSSPropertyFilter.regexpSource
+ * @param {String} selectorPrefix see CSSPropertyFilter.selectorPrefix
+ * @param {String} selectorSuffix see CSSPropertyFilter.selectorSuffix
  * @constructor
  * @augments ElemHideBase
  */
-function CSSPropertyFilter(text, domains, selector)
+function CSSPropertyFilter(text, domains, selector, regexpSource,
+  selectorPrefix, selectorSuffix)
 {
   ElemHideBase.call(this, text, domains, selector);
 
-  let properties;
-  [properties, , this.regexpSource] = selector.match(Filter.csspropertyRegExp);
-  [this.selectorPrefix, this.selectorSuffix] = selector.split(properties);
+  this.regexpSource = regexpSource;
+  this.selectorPrefix = selectorPrefix;
+  this.selectorSuffix = selectorSuffix;
 }
 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
+   * CSS properties - for delayed creation of the regexpString 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.

+   * Raw regular expression string to be used when testing CSS properties
+   * against this filter
+   * @type String
+   */
+  get regexpString()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
-    let prop = Object.getOwnPropertyDescriptor(this, "regexp");
+    let prop = Object.getOwnPropertyDescriptor(this, "regexpString");
     if (prop)
       return prop.value;
 
-    let source = Filter.toRegExp(this.regexpSource);
-    let regexp = new RegExp(source, "");
-    Object.defineProperty(this, "regexp", {value: regexp});
+    let regexp = Filter.toRegExp(this.regexpSource);
+    Object.defineProperty(this, "regexpString", {value: regexp});
     return regexp;
   }
 };