Issue 3143 - Filter elements with :-abp-has()

chrome/content/elemHideEmulation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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/>.
  */
 
 /* globals filterToRegExp */
 
 "use strict";
 
 const abpSelectorRegexp = /:-abp-([\w-]+)\(/i;
 
+let reportError = () => {};
+
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
 
   let selectors = [];
   let start = 0;
   let level = 0;
   let sep = "";
 
@@ skipping 108 matching lines @@
   let match = abpSelectorRegexp.exec(selector);
   if (!match)
     return [new PlainSelector(selector)];
 
   let selectors = [];
   if (match.index > 0)
     selectors.push(new PlainSelector(selector.substr(0, match.index)));
 
   let startIndex = match.index + match[0].length;
   let content = parseSelectorContent(selector, startIndex);
-  if (content == null)

[Sebastian Noack, 2017/06/08 08:28:47]

Nit: Given that parseSelectorContent() either returns and object or null,
perhaps `!content` is more appropriate here rather than `content == null`.

[hub, 2017/06/08 15:45:47]

Done.

-  {
-    console.error(new SyntaxError("Failed parsing AdBlock Plus " +

[Sebastian Noack, 2017/06/08 08:28:48]

Nit: It's Adblock Plus (with lower case "B"). But perhaps we should avoid
mentioning the product name in error messages in the first place, in order to
avoid confusion in other products that are based on this code. (Same below.)

[hub, 2017/06/08 15:45:48]

Maybe we could change this to "Failed parsing content blocker selector". It is
neutral.

[Felix Dahlke, 2017/06/09 09:02:33]

Well, we're actually parsing what we call an "ABP selector" (i.e. the :abp-*
pseudo-classes) here, so I think we should call it that in the error message:
Either "Adblock Plus selector" or "ABP selector". I personally prefer the latter
since it seems easier to make the connection to the :abp-* pseudo-classes that
way.

Various products with different names use this code, but it's still an "ABP
selector", similar to how we and others typically refer to the filter list
syntax as something to the effect of "ABP-style filters".

-                                  `selector ${selector}, didn't ` +
-                                  "find closing parenthesis."));
+  if (!content)
+  {
+    reportError(new SyntaxError("Failed to parse Adblock Plus " +
+                                `selector ${selector}, ` +
+                                "due to unmatched parentheses."));
     return null;
   }
   if (match[1] == "properties")
     selectors.push(new PropsSelector(content.text));
   else if (match[1] == "has")
   {
     let hasSelector = new HasSelector(content.text);
     if (!hasSelector.valid())
       return null;
     selectors.push(hasSelector);
   }
   else
   {
     // this is an error, can't parse selector.
-    console.error(new SyntaxError("Failed parsing AdBlock Plus " +
-                                  `selector ${selector}, invalid ` +
-                                  `pseudo-class -abp-${match[1]}().`));
+    reportError(new SyntaxError("Failed to parse Adblock Plus " +
+                                `selector ${selector}, invalid ` +
+                                `pseudo-class :-abp-${match[1]}().`));
     return null;
   }
 
   let suffix = parseSelector(selector.substr(content.end + 1));
   if (suffix == null)
     return null;
 
   selectors.push(...suffix);
 
   return selectors;
 }
 
 /** Stringified style objects
  * @typedef {Object} StringifiedStyle
- * @property {string} style CSS style represented a string.

[Wladimir Palant, 2017/06/08 13:12:40]

This doesn't seem to be quite English to me, also not very helpful. "sorted and
joined list of CSS properties" maybe? Note that this isn't a full sentence, so
there shouldn't be a period at the end.

[hub, 2017/06/08 15:45:47]

Acknowledged.

- * @property {string[]} subSelectors selectors for the rule.

[Wladimir Palant, 2017/06/08 13:12:39]

"selectors that the CSS properties apply to"?

[hub, 2017/06/08 15:45:46]

Acknowledged.

+ * @property {string} style CSS style represented by a string.
+ * @property {string[]} subSelectors selectors the CSS properties apply to.
  */
 
 /**
- * Turn a CSSStyleRule into a StringifiedStyle

[Wladimir Palant, 2017/06/08 13:12:39]

You have the types listed below, no need to repeat them. "Produces a string
representation of a stylesheet entry." maybe? Note that this is a sentence, so
it should end with a period.

[hub, 2017/06/08 15:45:47]

Acknowledged.

+ * Produce a string representation of the stylesheet entry.
  * @param {CSSStyleRule} rule the CSS style rule.
  * @return {StringifiedStyle} the stringified style.
  */
 function stringifyStyle(rule)
 {
   let styles = [];
   for (let i = 0; i < rule.style.length; i++)
   {
     let property = rule.style.item(i);
     let value = rule.style.getPropertyValue(property);
     let priority = rule.style.getPropertyPriority(property);
-    styles.push(property + ": " + value + (priority ? " !" + priority : "") +

[Sebastian Noack, 2017/06/08 08:28:47]

Nit: If you'd use a template literal here you woudnl't have to wrap this line:

  `${property}: ${value}${priority ? " !" + priority : ""};`

[hub, 2017/06/08 15:45:46]

This code was simply moved and written pre-ES6. I'll use a template string.

-                ";");
+    styles.push(`${property}: ${value}${priority ? " !" + priority : ""};`);
   }
   styles.sort();
   return {
     style: styles.join(" "),
     subSelectors: splitSelector(rule.selectorText)
   };
 }
 
 function* evaluate(chain, index, prefix, subtree, styles)
 {
@@ skipping 149 matching lines @@
 
       for (let rule of rules)
       {
         if (rule.type != rule.STYLE_RULE)
           continue;
 
         cssStyles.push(stringifyStyle(rule));
       }
     }
 
+    let {document} = this.window;
     for (let pattern of this.patterns)
     {
       for (let selector of evaluate(pattern.selectors,
                                     0, "", document, cssStyles))
       {
         if (!pattern.selectors.some(s => s.requiresHiding))
         {
           selectors.push(selector);
           selectorFilters.push(pattern.text);
         }
@@ skipping 16 matching lines @@
   {
     let stylesheet = event.target.sheet;
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   apply()
   {
     this.getFiltersFunc(patterns =>
     {
+      let oldReportError = reportError;
+      reportError = error => this.window.console.error(error);
+
       this.patterns = [];
       for (let pattern of patterns)
       {
         let selectors = parseSelector(pattern.selector);
         if (selectors != null && selectors.length > 0)
           this.patterns.push({selectors, text: pattern.text});
       }
 
       if (this.patterns.length > 0)
       {
         let {document} = this.window;
         this.addSelectors(document.styleSheets);
         document.addEventListener("load", this.onLoad.bind(this), true);
       }
+      reportError = oldReportError;
     });
   }
 };