Issue 6193 - Allow element hiding emulation exceptions

lib/filterValidation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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/>.
  */
 
 /** @module filterValidation */
 
 "use strict";
 
 const {Filter, InvalidFilter, ElemHideBase, ElemHideException,
        ElemHideEmulationFilter} = require("filterClasses");
-const {Utils} = require("utils");
 
 /**
  * An error returned by
  * {@link module:filterValidation.parseFilter parseFilter()} or
  * {@link module:filterValidation.parseFilters parseFilters()}
  * indicating that a given filter cannot be parsed,
  * contains an invalid CSS selector or is a filter list header.
  *
  * @param {string} type See documentation in the constructor below.
  * @param {Object} [details] Contains the "reason" and / or "selector"
@@ skipping 31 matching lines @@
 
   /**
    * Returns a detailed translated error message.
    *
    * @return {string}
    */
   toString()
   {
     let message;
     if (this.reason)
-      message = Utils.getString(this.reason);
+      message = browser.i18n.getMessage(this.reason);
     else
     {
       message = browser.i18n.getMessage(
         this.type.replace(/-/g, "_"),
         "selector" in this ? "'" + this.selector + "'" : null
       );
     }
 
     if (this.lineno)
     {
@@ skipping 19 matching lines @@
   }
   catch (e)
   {
     return false;
   }
   return true;
 }
 
 function isValidFilterSelector(filter)
 {
-  // Only ElemHideBase has selectors

[kzar, 2018/04/05 15:00:27]

Perhaps split these up a bit? Something like this (untested)?

if (!(filter instanceof ElemHideBase))
  return true;

// We don't yet check to see if element hiding emulation filters
// have valid selectors yet. See issue xxx.
if (filter instanceof ElemHideEmulationFilter)
  return true;

// Since the #@# syntax is used for exception to both element hiding
// and element hiding emulation filters we now need to make sure only
// the former are checked.
if ((filter instanceof ElemHideException) && filter.selector.includes(":-abp-"))
  return true;

return isValidCSSSelector(filter.selector);

[hub, 2018/04/05 17:04:41]

Done.

-  // We don't check the syntax of ElemHideEmulationFilter yet
-  if (!(filter instanceof ElemHideBase)
-      || (filter instanceof ElemHideEmulationFilter))

[kzar, 2018/04/05 15:00:27]

Nit: Parenthesis around the second clause seem redundant.
Nit: We would normally put the `||` at the end of the previous line. (Did this
pass linting?)
Nit: Since the clause spans multiple lines please use braces for the body. (Same
below.)

[hub, 2018/04/05 17:04:41]

Done.

+  // Only ElemHideBase has selectors.
+  if (!(filter instanceof ElemHideBase))
     return true;

[kzar, 2018/04/05 15:00:27]

Nit: I think a newline between these cases would look cleaner.

[hub, 2018/04/05 17:04:41]

Done.

-  // If it ElemHideException, don't check if it is
-  // for an EmulationFilter
-  if ((filter instanceof ElemHideException) &&
-      (filter.selector.indexOf(":-abp-") != -1))

[kzar, 2018/04/05 15:00:27]

Is this check good enough now that we support :has?

[hub, 2018/04/05 17:04:41]

This idea is that like above if we have one of the ":-abp-" pseudo selector we
don't check if this is a valid CSS selector, and assume it is fine. I don't even
know how I could let the comment above. I'll rephrase it properly.

In the future we should have a proper syntax checking all the way, but right
now, this is just to prevent the filter from being rejected when the user add
it.

[kzar, 2018/04/06 10:05:44]

Acknowledged.

+
+  // We don't check the syntax of ElemHideEmulationFilter yet.
+  if (filter instanceof ElemHideEmulationFilter)
     return true;
+
+  // If it is an ElemHideException, and it has an extended CSS
+  // selector we don't validate and assume it is valid.
+  if (filter instanceof ElemHideException &&
+      filter.selector.includes(":-abp-"))
+  {
+    return true;
+  }
 
   return isValidCSSSelector(filter.selector);
 }
 
 /**
  * @typedef ParsedFilter
  * @property {?Filter} [filter]
  *   The parsed filter if it is valid. Or null if the given string is empty.
  * @property {FilterParsingError} [error]
  *   See {@link module:filterValidation~FilterParsingError FilterParsingError}
@@ skipping 62 matching lines @@
 
     if (error)
     {
       error.lineno = i + 1;
       errors.push(error);
     }
   }
 
   return {filters, errors};
 };