Issue 6296 - Handle relative prefix in :-abp-has()

lib/content/elemHideEmulation.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 117 matching lines @@
 {
   let elements = null;
   try
   {
     elements = all ? subtree.querySelectorAll(selector) :
       subtree.querySelector(selector);
     scopeSupported = true;
   }
   catch (e)
   {
+    // Edge doesn't support ":scope"
     scopeSupported = false;

[Manish Jethani, 2018/01/30 05:53:41]

Maybe we should add that comment here saying that Edge does not support :scope

[hub, 2018/01/30 16:03:57]

Done.

   }
   return elements;
 }
 
 /**
- * Query selector. If it is relative, will try :scoped.

[Manish Jethani, 2018/01/30 05:53:41]

You meant ":scope" (without the D)?

[hub, 2018/01/30 16:03:57]

Done.

+ * Query selector. If it is relative, will try :scope.
  * @param {Node} subtree the element to query selector
  * @param {string} selector the selector to query
- * @param {bool} all True to perform querySelectorAll()

[Manish Jethani, 2018/01/30 05:53:41]

Since all is optional and defaults to false, I guess this should be
"[all=false]".

Also I think "true" should not be capitalized.

[hub, 2018/01/30 16:03:57]

Done.

- * @returns {Node|NodeList} result of the query. null in case of error.

[Manish Jethani, 2018/01/30 05:53:41]

Since this can return null I think it should be "{?(Node|NodeList)}".

[lainverse, 2018/01/30 15:28:29]

BTW, wouldn't it be better to return empty array when all == true? This way
extra check if scopedQuerySelectorAll() returned something won't be necessary.

I mean it would be possible to use it like this

for (let element of scopedQuerySelectorAll(subtree, actualPrefix))

instead of

let elements = scopedQuerySelectorAll(subtree, actualPrefix);
if (elements)
{
  for (let element of elements)
...


That is similar to original querySelectorAll which always returns NodeList even
when it's empty.

[hub, 2018/01/30 16:03:57]

Done.

+ * @param {bool} [all=false] true to perform querySelectorAll()
+ * @returns {?(Node|NodeList)} result of the query. null in case of error.
  */
 function scopedQuerySelector(subtree, selector, all)
 {
-  if (relativeSelectorRegexp.test(selector))
+  if (selector[0] == ">")
   {
     selector = ":scope" + selector;
     if (scopeSupported)
     {
       return all ? subtree.querySelectorAll(selector) :
         subtree.querySelector(selector);
     }
     if (scopeSupported == null)
       return tryQuerySelector(subtree, selector, all);
     return null;
@@ skipping 40 matching lines @@
    * @param {string} prefix the prefix for the selector.
    * @param {Node} subtree the subtree we work on.
    * @param {StringifiedStyle[]} styles the stringified style objects.
    */
   *getSelectors(prefix, subtree, styles)
   {
     yield [prefix + this._selector, subtree];
   }
 };
 
 const incompletePrefixRegexp = /[\s>+~]$/;

[lainverse, 2018/01/30 18:07:28]

Since that other regexp were replaced with string comparison it might be a good
idea to replace this one too:

{
  let u = [];
  let n = 10000000;
  let c = 0;
  let r = /[\s>+~]$/;
  let incomplete = [' ','>','+','~'];
  let isIncompleteArr = c => incomplete.includes(c);
  let isIncompleteStr = c => c == ' ' || c == '>' || c == '+' || c == '~';
  for (let i = 0; i < n; i++)
  {
    if (Math.random() < .1)
      u.push("foo bar" + incomplete[Math.floor(Math.random()*4)]);
    else
      u.push("foo bar");
  }
  let s = performance.now();
  for (let i = 0; i < n; i++)
  {
    //if (r.test(u[i]))
    // if (isIncompleteArr(u[i][u[i].length-1]))
    if (isIncompleteStr(u[i][u[i].length-1]))
      c++;
  }
  let t = (performance.now() - s) | 0;
  console.log(c + " matches found in " + t + "ms");
}

In my tests function isIncompleteArr performs on par with regexp and
isIncompleteStr ~50ms faster both in Firefox and Chrome. Downside: I only check
for normal space character in both functions instead of \s as in the original
regexp. This should work fine if ABP normalize spaces in filters during import.

[Manish Jethani, 2018/01/30 20:23:20]

If we do this then I think that it should be a separate patch.

-const relativeSelectorRegexp = /^>/;

[Manish Jethani, 2018/01/30 06:13:06]

So I ran this on Chrome and Firefox:

{
  let u = [];
  let n = 10000000;
  let c = 0;
  let r = /^>/;
  for (let i = 0; i < n; i++)
  {
    if (Math.random() < .1)
      u.push(">foo bar");
    else
      u.push("foo bar");
  }
  let s = performance.now();
  for (let i = 0; i < n; i++)
  {
    // if (r.test(u[i]))
    // if (u[i].charCodeAt(0) == 62)
    if (u[i][0] == ">")
      c++;
  }
  let t = (performance.now() - s) | 0;
  console.log(c + " matches found in " + t + "ms");
}

It seems comparing the first character directly is the fastest on Firefox,
whereas on Chrome the fastest is to use String.charCodeAt. On both platforms the
regular expression is the slowest, and it's significantly slower.

I think we should go for just string comparison here.

[hub, 2018/01/30 16:03:57]

Done.

 
 function HasSelector(selectors)
 {
   this._innerSelectors = selectors;
 }
 
 HasSelector.prototype = {
   requiresHiding: true,
 
   get dependsOnStyles()
@@ skipping 421 matching lines @@
           characterData: true,
           subtree: true
         }
       );
       this.document.addEventListener("load", this.onLoad.bind(this), true);
     }
   }
 };
 
 exports.ElemHideEmulation = ElemHideEmulation;