Issue 6538, 6781 - Implement support for snippet filters

lib/filterClasses.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
@@ skipping 168 matching lines @@
   text = text.replace(/[^\S ]+/g, "");
 
   // Don't remove spaces inside comments
   if (/^ *!/.test(text))
     return text.trim();
 
   // Special treatment for content filters, right side is allowed to contain
   // spaces
   if (Filter.contentRegExp.test(text))
   {
-    let [, domains, separator, script] = /^(.*?)(#[@?$]?#?)(.*)$/.exec(text);
-    return domains.replace(/ +/g, "") + separator + script.trim();
+    let [, domains, separator, body] = /^(.*?)(#[@?$]?#?)(.*)$/.exec(text);
+    return domains.replace(/ +/g, "") + separator + body.trim();
   }
 
   // For most regexp filters we strip all spaces, but $csp filter options
   // are allowed to contain single (non trailing) spaces.
   let strippedText = text.replace(/ +/g, "");
   if (!strippedText.includes("$") || !/\bcsp=/i.test(strippedText))
     return strippedText;
 
   let optionsMatch = Filter.optionsRegExp.exec(strippedText);
   if (!optionsMatch)
@@ skipping 789 matching lines @@
 
 WhitelistFilter.prototype = extend(RegExpFilter, {
   type: "whitelist"
 });
 
 /**
  * Base class for content filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} [domains] Host names or domains the filter should be
  *                           restricted to
- * @param {string} script    Script that should be executed
+ * @param {string} body      The body of the filter
  * @constructor
  * @augments ActiveFilter
  */
-function ContentFilter(text, domains, script)
+function ContentFilter(text, domains, body)
 {
   ActiveFilter.call(this, text, domains || null);
 
-  this.script = script;
+  this.body = body;
 }
 exports.ContentFilter = ContentFilter;
 
 ContentFilter.prototype = extend(ActiveFilter, {
   /**
    * @see ActiveFilter.domainSeparator
    */
   domainSeparator: ",",
 
   /**
-   * Script that should be executed

[kzar, 2018/07/10 12:53:22]

This comment seems misleading, I think it should be clearer that the value also
might be a CSS selector.

[Manish Jethani, 2018/07/10 18:29:20]

This is kinda overridden in the subclass as a new property called "selector"
(what it was called previously). The way I think of it, ContentFilter is generic
and it always runs a "script" on the document; ElemHideBase is a special case
where the "script" is a CSS selector.

[kzar, 2018/07/10 18:53:59]

To me a CSS selector is not a script (nor code), which is why I think this name
is misleading. Also I don't think of element hiding filters to be a special case
of content filters, like you mention they're by far the most common.

+   * The body of the filter
    * @type {string}
    */
-  script: null

[kzar, 2018/07/10 12:53:22]

"script" seems a misleading name, how about "body"? (We should also update some
of the comments and parameter names to match.)

[Manish Jethani, 2018/07/10 18:29:20]

This was originally called "code", then I renamed it to "script". "script" seems
OK to me, but I don't really care too much what it's called. What do you think,
"code" or "body"?

[kzar, 2018/07/10 18:53:59]

I prefer "body" over "script" and "code".

[Manish Jethani, 2018/07/11 13:04:25]

Done.

+  body: null
 });
 
 /**
  * Creates a content filter from a pre-parsed text representation
  *
  * @param {string} text         same as in Filter()
  * @param {string} [domains]
  *   domains part of the text representation
  * @param {string} [type]
- *   rule type, either empty or @ (exception) or ? (emulation rule) or

[kzar, 2018/07/10 12:53:22]

Please could you improve this part of the comment? How about this?

The rule type, either:
 - "" for an element hiding filter
 - "@" for an exception filter
 - "?" for an element hiding emulation filter
 - "$" for a snippet filter

I don't care too much about the formatting anyway, so long as it's a bit easier
to grok.

[Manish Jethani, 2018/07/11 13:04:24]

Done.

[Manish Jethani, 2018/07/11 13:16:31]

By the way, in case you're wondering if it shouldn't be called "content
exception" rather than "element hiding exception", I have thought about this. In
the initial version these exceptions won't apply to snippet filters. When we get
around to it finally, the class should also be renamed to ContentException and
should be moved one level up to extend directly from ContentFilter. But I'm not
fully sure about this. Maybe snippet filters should have their own exceptions
like "#$@#"? Something to think about.

[kzar, 2018/07/11 16:33:12]

Yea, this is something I asked you about on one of the other reviews. I
disagree, I think #@# exceptions should apply to snippet filters too. Unless I'm
mistaken "example.com#@#.some_class" isn't going to matter to a snippet filter,
but it would be logical to expect "example.com#@#foo arg" to work.

Sebastian/Sergz what do you guys think?

[Manish Jethani, 2018/07/11 16:42:46]

Yeah, I too think that those exceptions could just apply to snippet filters as
well. We will have to update the code and the documentation, that's all.

One challenge with the exceptions was that the selector property was being
escaped. This is mostly resolved now, because we have a separate
code/script/body property (unescaped) in the superclass, which we should use for
matching. The selector property should be used only to get the actual (escaped)
CSS selector to inject.

[Sebastian Noack, 2018/07/11 16:47:30]

So far for misusing element-hiding-like syntax for snippets. Just another
example why this analogy lacks (and causes confusion). But well, assuming it's
now established that we go with this obscure syntax, I agree with Manish here
that #@# exceptions rules probably shouldn't apply to snippet filters.

The problem is "x y" could either be a CSS selector targeting an element with
the name "y" inside another element with the name "x", or it could mean snippet
"x" with argument "y".

[Manish Jethani, 2018/07/11 16:55:05]

I'm not sure I agree with that, but since we are undecided, and we don't need
exceptions to work in the initial version, we can postpone the decision.
I highly doubt that this will be a problem in practice, but we could also just
have a naming convention for snippet functions so they never clash with CSS
selectors. For example simply following the practice of surrounding all snippet
function names with single quotes, in the snippet filter itself (which is
supported by the syntax), would be enough.

[Manish Jethani, 2018/07/11 17:12:10]

To give some background on this, the initial version is an "alpha" or even
"pre-alpha" if you will. It will only be used by our own filter list authors (we
will discard snippet filters in any other filter lists [1]). This means the
syntax can be changed, everything can be changed easily. We should really stop
worrying about this right now.

When we have developed the feature fully and we know that it is giving results
(which we can only know if we try it out in production), then we can flip the
switch and make it available to all filter list authors. Until then, we have
time to figure out the details.

[1]: https://codereview.adblockplus.org/29824589/

[kzar, 2018/07/11 17:17:51]

OK, seems the vote is #@# exception filters shouldn't apply to snippet filters
for now. Please just add a note about this in the issue to explain the
situation, and ideally link to an issue about adding the #@$# or similiar
syntax.

[kzar, 2018/07/11 17:17:51]

Well I was already in the process of replying... I can only do a review so fast.

- *   $ (snippet)
- * @param {string} script
- *   script part of the text representation, either a CSS selector or a snippet
+ *   rule type, either:
+ *     <li>"" for an element hiding filter</li>

[Manish Jethani, 2018/07/11 13:04:26]

Using just "-" doesn't work in the HTML version, but we can use <li> instead.

We should really switch to Markdown in the comments.

[kzar, 2018/07/11 17:17:52]

Acknowledged.

+ *     <li>"@" for an element hiding exception filter</li>
+ *     <li>"?" for an element hiding emulation filter</li>
+ *     <li>"$" for a snippet filter</li>
+ * @param {string} body
+ *   body part of the text representation, either a CSS selector or a snippet
  *   script
  * @return {ElemHideFilter|ElemHideException|
  *          ElemHideEmulationFilter|SnippetFilter|InvalidFilter}
  */
-ContentFilter.fromText = function(text, domains, type, script)
+ContentFilter.fromText = function(text, domains, type, body)
 {
   // We don't allow content filters which have any empty domains.
   // Note: The ContentFilter.prototype.domainSeparator is duplicated here, if
   // that changes this must be changed too.
   if (domains && /(^|,)~?(,|$)/.test(domains))
     return new InvalidFilter(text, "filter_invalid_domain");
 
   if (type == "@")
-    return new ElemHideException(text, domains, script);
+    return new ElemHideException(text, domains, body);
 
   if (type == "$")
-    return new SnippetFilter(text, domains, script);
+    return new SnippetFilter(text, domains, body);
 
   if (type == "?")
   {
     // Element hiding emulation filters are inefficient so we need to make sure
     // that they're only applied if they specify active domains
     if (!/,[^~][^,.]*\.[^,]/.test("," + domains))
       return new InvalidFilter(text, "filter_elemhideemulation_nodomain");
 
-    return new ElemHideEmulationFilter(text, domains, script);
-  }
-
-  return new ElemHideFilter(text, domains, script);
+    return new ElemHideEmulationFilter(text, domains, body);
+  }
+
+  return new ElemHideFilter(text, domains, body);
 };
 
 /**
  * Base class for element hiding filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} [domains] see {@link ContentFilter ContentFilter()}
  * @param {string} selector  CSS selector for the HTML elements that should be
  *                           hidden
  * @constructor
  * @augments ContentFilter
  */
 function ElemHideBase(text, domains, selector)
 {
   ContentFilter.call(this, text, domains, selector);
 }
 exports.ElemHideBase = ElemHideBase;
 
 ElemHideBase.prototype = extend(ContentFilter, {
   /**
    * CSS selector for the HTML elements that should be hidden
    * @type {string}
    */
   get selector()
   {
     // Braces are being escaped to prevent CSS rule injection.
-    return this.script.replace("{", "\\7B ").replace("}", "\\7D ");
+    return this.body.replace("{", "\\7B ").replace("}", "\\7D ");
   }
 });
 
 /**
  * Class for element hiding filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} [domains]  see {@link ElemHideBase ElemHideBase()}
  * @param {string} selector see {@link ElemHideBase ElemHideBase()}
  * @constructor
  * @augments ElemHideBase
@@ skipping 52 matching lines @@
  * @constructor
  * @augments ContentFilter
  */
 function SnippetFilter(text, domains, script)
 {
   ContentFilter.call(this, text, domains, script);
 }
 exports.SnippetFilter = SnippetFilter;
 
 SnippetFilter.prototype = extend(ContentFilter, {
-  type: "snippet"
-});
+  type: "snippet",
+
+  /**
+   * Script that should be executed
+   * @type {string}
+   */
+  get script()
+  {
+    return this.body;
+  }
+});