Issue 6592 - Implement $rewrite filter option

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 878 matching lines @@
  * @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()
  * @param {boolean} [collapse]
  *   defines whether the filter should collapse blocked content, can be null
  * @param {string} [csp]
  *   Content Security Policy to inject when the filter matches
- * @param {string} [rewrite]
- *   The rewrite expression

[kzar, 2018/05/15 14:08:58]

Mind making this comment useful? (Same below.)

[hub, 2018/05/15 16:16:31]

Done.

[kzar, 2018/05/16 09:22:40]

Thanks, looking way better now.

+ * @param {?string} [rewrite]
+ *   The (optional) rule specifying how to rewrite the URL. See
+ *   BlockingFilter.prototype.rewrite.
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
                         thirdParty, sitekeys, collapse, csp, rewrite)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 
   this.collapse = collapse;
@@ skipping 12 matching lines @@
    */
   collapse: null,
 
   /**
    * Content Security Policy to inject for matching requests.
    * @type {?string}
    */
   csp: null,
 
   /**
-   * The rewrite expression
+   * The rule specifying how to rewrite the URL.
+   * The syntax is similar to the one of String.prototype.replace().
    * @type {?string}
    */
   rewrite: null,
 
   /**
    * Rewrites an URL.
    * @param {string} url the URL to rewrite
    * @return {string} the rewritten URL, or the original in case of failure
    */
   rewriteUrl(url)
   {
     try
     {
       let rewrittenUrl = new URL(url.replace(this.regexp, this.rewrite), url);

[kzar, 2018/05/17 12:56:06]

I forgot to mention before, I thought how you did this part was clever. Just
reusing the existing generated regexp.

[hub, 2018/05/17 12:58:39]

thanks!

       if (rewrittenUrl.origin == new URL(url).origin)
         return rewrittenUrl.href;
     }
     catch (e)
     {
     }
 
     return url;

[kzar, 2018/05/15 14:24:57]

Why do we return `url` on failure? Shouldn't we return `false` or similar
instead? That would be cheaper to check in the onBeforeRequest listener. (Yea,
we'd also have to tweak the above logic to check that `url != rewrittenUrl`.)

[Sebastian Noack, 2018/05/15 14:40:34]

Even if no error is thrown and no origin change was detected, the URL might be
the same after the substitution. If we distinguish between those scenarios here,
we'd also have to handle them separately in the calling code.

[kzar, 2018/05/15 14:44:23]

Not if we tweak the above logic, so we don't return the rewritten URL if either
the origins don't match or if the URLs are identical.

[Sebastian Noack, 2018/05/15 14:51:02]

Sure, we could add another check here, in order to only return a string if it
differs from the original URL, and then in the calling code check for null
instead of checking whether the value changed. But I don't see how that makes
anything simpler (or faster).

[kzar, 2018/05/15 14:53:58]

Well it seems pointless to return the existing URL if the origin didn't match
(or replace threw) only to then compare that URL with the existing one
afterwards. But I guess it's not expected to be too common a case so I guess I
don't insist.

[Manish Jethani, 2018/05/15 15:18:12]

These are not the only cases in which the rewrite doesn't happen (the above
examples are failures), it could also be that there's a filter like
/(trackingId=)\d+/$rewrite=$10 (replace any tracking ID with 0), in which case
it would "fail" for "trackingId=0" (technically successful but nothing changed).
I think explicitly comparing the output with the input is a bit weird for this
case.

[hub, 2018/05/15 16:16:31]

The original idea is to not block the request in case of error. A gentle
failure.

   }
 });
 
 /**
  * Class for whitelist 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()
@@ skipping 145 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });