Issue 4394 - Create a filter class for element hiding emulation filters [WIP]

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -91,21 +91,28 @@
  */
 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
+ * Regular expression that element hiding filters using property selectors
+ * should match
  * Properties must not contain " or '
  * @type RegExp
  */
-Filter.csspropertyRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/;
+Filter.propertyselectorRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/;
+/**
+ * Regular expression that element hiding filters using the :has pseudo-class
+ * should match
+ * @type RegExp
+ */
+Filter.haspseudoclassRegExp = /:has\(/;

[Felix Dahlke, 2016/09/23 16:41:59]

Didn't want to put too much energy into this regex at this stage, but I'll
revisit it.

[Felix Dahlke, 2016/10/05 10:08:26]

In-person comment from Wladimir: We probably don't need a regex match here, we
cannot use regex to parse the filter anyway. So we can just do a string match.

Idea from me as a result of this: We just do a string check for -abp-properties
as well, we move the parsing into the CPF content script for now.

[kzar, 2016/10/05 11:58:15]

Acknowledged.

 
 /**
  * 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}
  */
@@ -878,17 +885,17 @@
  * 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 {Boolean} isException exception rule indicator
  * @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|CSSPropertyFilter|InvalidFilter}
+ * @return {ElemHideFilter|ElemHideException|ElemHideEmulationFilter|InvalidFilter}
  */
 ElemHideBase.fromText = function(text, domain, isException, tagName, attrRules, selector)
 {
   if (!selector)
   {
     if (tagName == "*")
       tagName = "";
 
@@ -922,27 +929,42 @@
       selector = tagName + additional;
     else
       return new InvalidFilter(text, "filter_elemhide_nocriteria");
   }
 
   if (isException)
     return new ElemHideException(text, domain, selector);
 
-  let match = Filter.csspropertyRegExp.exec(selector);
-  if (match)
+  let hasSelectorUsed = Filter.haspseudoclassRegExp.test(selector);

[Felix Dahlke, 2016/09/23 16:41:59]

This code block is all about checking if the element hiding rule needs to be
emulated. Given the increasing complexity, I'd love to move this code into a
dedicated function (e.g. in ElemHideEmulationFilter). But for now I'm leaving it
here to make it easier to see what I've changed.

[kzar, 2016/10/05 11:58:14]

Yea I agree, if you don't move this into a separate function at least put a
comment for the block of code explaining the general idea.

[Felix Dahlke, 2016/11/03 16:14:13]

The code is quite a bit shorter now that I reduced this to detection logic and
moved all the parsing stuff to the content script. So I figured it's fine
without comment/function for now. Have a look at the new code and let me know
what you think.

+  let propertySelectorMatch = Filter.propertyselectorRegExp.exec(selector);

[Felix Dahlke, 2016/09/23 16:41:59]

I'm not really happy that we need to run every feature regex on every element
hiding filter this way. Ideas welcome.

[kzar, 2016/09/25 14:52:06]

Could we have a regexp which matches all element hiding emulation filters?

[Felix Dahlke, 2016/09/30 10:22:26]

Yeah, I think that should be possible. But it would be incredibly complex, as we
add more features, hence I much prefer separate regexps for readability's sake.
But I'm planning to run some performance tests to see how bad it really is, then
we can still change that.

+  if (hasSelectorUsed || propertySelectorMatch)
   {
-    // CSS property filters are inefficient so we need to make sure that
-    // they're only applied if they specify active domains
+    // Element hiding emulation 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, "filter_cssproperty_nodomain");
+      return new InvalidFilter(text, "filter_elemhideemulation_nodomain");

[kzar, 2016/09/25 15:51:12]

(I guess we'll have to update this string in adblockplus and then update the
adblockplus dependency in adblockpluschrome at the same time we make the other
changes in those repositories.)

[Felix Dahlke, 2016/09/30 10:22:26]

Yup, I was planning to roll out my adblockplus changes first, then
adblockpluschrome. I wonder whether we can just rename a translation string
without invalidating the existing translations on Crowdin - do you have
experience with that?

[Felix Dahlke, 2016/10/05 10:08:27]

In-person comment from Wladimir: It's a different string anyway, there's no
reason to save the existing translations.

 
-    return new CSSPropertyFilter(text, domain, selector, match[2],
-      selector.substr(0, match.index),
-      selector.substr(match.index + match[0].length));
+    let regexpSource, selectorPrefix, selectorSuffix;
+    if (propertySelectorMatch)
+    {
+      regexpSource = propertySelectorMatch[2];

[Felix Dahlke, 2016/09/23 16:41:59]

These properties are only necessary for the CSSPropertyFilters content script.
Seems easiest to leave them here for now, since they will probably not be
necessary anymore once we tackle https://issues.adblockplus.org/ticket/3143.
OTOH, it's a bit messy to leave it this way until that issue is tackled. What do
you think?

[kzar, 2016/09/25 14:52:07]

But we still need the selector suffix and prefix for :has filters, so why would
we remove these? I guess I'm missing something.

[Felix Dahlke, 2016/09/30 10:22:26]

Well, a filter can have both :has and property selectors - multiple times even,
in theory. So I believe this is going to change. But "believe" is the right
word, I'm not 100% sure how things are going to end up looking...

[kzar, 2016/10/05 11:58:15]

OK fair enough, me neither to be honest.

+      selectorPrefix = selector.substr(0, propertySelectorMatch.index);
+      selectorSuffix = selector.substr(propertySelectorMatch.index
+                                       + propertySelectorMatch[0].length);
+    }
+
+    let features = 0;
+    if (hasSelectorUsed)
+      features |= ElemHideEmulationFilter.featureMap.HAS_PSEUDO_CLASS;
+    if (propertySelectorMatch)
+      features |= ElemHideEmulationFilter.featureMap.PROPERTY_SELECTOR;
+
+    return new ElemHideEmulationFilter(text, domain, selector, features,
+      regexpSourcex, selectorPrefix, selectorSuffix);
   }
 
   return new ElemHideFilter(text, domain, selector);
 };
 
 /**
  * Class for element hiding filters
  * @param {String} text see Filter()
@@ -975,39 +997,48 @@
 }
 exports.ElemHideException = ElemHideException;
 
 ElemHideException.prototype = extend(ElemHideBase, {
   type: "elemhideexception"
 });
 
 /**
- * Class for CSS property filters
+ * Class for element hiding emulation filters
  * @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
+ * @param {Integer} features      see ElemHideEmulationFilter.features
+ * @param {String} regexpSource   see ElemHideEmulationFilter.regexpSource
+ * @param {String} selectorPrefix see ElemHideEmulationFilter.selectorPrefix
+ * @param {String} selectorSuffix see ElemHideEmulationFilter.selectorSuffix
  * @constructor
  * @augments ElemHideBase
  */
-function CSSPropertyFilter(text, domains, selector, regexpSource,
-  selectorPrefix, selectorSuffix)
+function ElemHideEmulationFilter(text, domains, selector, features,
+  regexpSource, selectorPrefix, selectorSuffix)
 {
   ElemHideBase.call(this, text, domains, selector);
 
+  this.features = features;
   this.regexpSource = regexpSource;
   this.selectorPrefix = selectorPrefix;
   this.selectorSuffix = selectorSuffix;
 }
-exports.CSSPropertyFilter = CSSPropertyFilter;
+exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
-CSSPropertyFilter.prototype = extend(ElemHideBase, {
-  type: "cssproperty",
+ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
+  type: "elemhideemulation",
+
+  /**
+   * Features used in this filter, combination of values from
+   * ElemHideEmulationFilter.featureMap
+   * @type Integer
+   */
+  features: 0,
 
   /**
    * Expression from which a regular expression should be generated for matching
    * CSS properties - for delayed creation of the regexpString property
    * @type String
    */
   regexpSource: null,
   /**
@@ -1036,8 +1067,16 @@
     if (prop)
       return prop.value;
 
     let regexp = Filter.toRegExp(this.regexpSource);
     Object.defineProperty(this, "regexpString", {value: regexp});
     return regexp;
   }
 });
+
+/**
+ * Map of features supported by element hiding emulation filters
+ */
+ElemHideEmulationFilter.featureMap = {
+  PROPERTY_SELECTOR: 1,
+  HAS_PSEUDO_CLASS: 2
+};