Issue 29676761: Issue 6296 - Handle relative prefix in :-abp-has() (Closed)
|
Created:
Jan. 22, 2018, 9:20 p.m. by hub Modified:
Feb. 1, 2018, 3:37 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6296 - Handle relative prefix in :-abp-has()
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rework the query selector all #
Total comments: 4
Patch Set 3 : addressed comments #
Total comments: 11
Patch Set 4 : No longer use regexp for relative selector. #
Total comments: 2
MessagesTotal messages: 15
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:201: let elements = subtree.querySelectorAll(actualPrefix); As I understand if this fails it's still possible to make it work with makeSelector(subtree) + ' ' + actualPrefix (without :scope and make sure to use 'HTML' instead of ':root' in the makeSelector in the Edge). Also, as I understand, ':scope' won't break complete selectors, so if it's available then it should be safe to use it all the time and leave regexp check only for browsers which doesn't support it. Just check is it (and ':root') available with something like this beforeahead: function checkSelector(selector) { try { document.querySelector(selector); } catch (e) { return false; } return true; }; const isScopeAndRootAvailable = checkSelector(':scope') && checkSelector(':root'); And since we already know if scope and root are available then try-catch check could be omitted since selector should be valid in all browsers.
Would you be able to take a look at this one Manish?
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:197: if (relativeSelectorRegexp.test(actualPrefix)) If I understand this correctly, "+" and "~" don't make sense in any context, therefore they can be removed from relativeSelectorRegexp? https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:201: let elements = subtree.querySelectorAll(actualPrefix); On 2018/01/23 01:02:51, lainverse wrote: > As I understand if this fails it's still possible to make it work with > makeSelector(subtree) + ' ' + actualPrefix (without :scope and make sure to use > 'HTML' instead of ':root' in the makeSelector in the Edge). I agree this could probably be made to work on Edge, but maybe that should be a separate patch. https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:212: if (relativeSelectorRegexp.test(selector)) So this and the try..catch could be made generic and put into a function: let scopeSupported = null; function tryQueryScopeSelectorAll(element, selector) { let result = []; try { result = element.querySelectorAll(selector); scopeSupported = true; } catch (e) { scopeSupported = false; } return result; } function querySelectorAll(element, selector) { if (relativeSelectorRegexp.test(selector)) { selector = ":scope" + selector; if (scopeSupported) return element.querySelectorAll(selector); if (scopeSupported == null) return tryQueryScopeSelectorAll(element, selector); return []; } return element.querySelectorAll(selector); } Then we can call querySelectorAll from the rest of the code and get an array (possibly empty) of elements, and it'll only do the try..catch once. I don't think we should always prefix with ":scope" because we still need to support Edge for selectors that don't need it.
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:201: let elements = subtree.querySelectorAll(actualPrefix); On 2018/01/23 16:42:11, Manish Jethani wrote: > On 2018/01/23 01:02:51, lainverse wrote: > > As I understand if this fails it's still possible to make it work with > > makeSelector(subtree) + ' ' + actualPrefix (without :scope and make sure to > use > > 'HTML' instead of ':root' in the makeSelector in the Edge). > > I agree this could probably be made to work on Edge, but maybe that should be a > separate patch. Filed https://issues.adblockplus.org/ticket/6304
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:197: if (relativeSelectorRegexp.test(actualPrefix)) On 2018/01/23 16:42:11, Manish Jethani wrote: > If I understand this correctly, "+" and "~" don't make sense in any context, > therefore they can be removed from relativeSelectorRegexp? Done. https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHid... lib/content/elemHideEmulation.js:212: if (relativeSelectorRegexp.test(selector)) On 2018/01/23 16:42:11, Manish Jethani wrote: > So this and the try..catch could be made generic and put into a function: > > let scopeSupported = null; > > function tryQueryScopeSelectorAll(element, selector) > { > let result = []; > try > { > result = element.querySelectorAll(selector); > scopeSupported = true; > } > catch (e) > { > scopeSupported = false; > } > return result; > } > > function querySelectorAll(element, selector) > { > if (relativeSelectorRegexp.test(selector)) > { > selector = ":scope" + selector; > if (scopeSupported) > return element.querySelectorAll(selector); > if (scopeSupported == null) > return tryQueryScopeSelectorAll(element, selector); > return []; > } > return element.querySelectorAll(selector); > } > > Then we can call querySelectorAll from the rest of the code and get an array > (possibly empty) of elements, and it'll only do the try..catch once. > > I don't think we should always prefix with ":scope" because we still need to > support Edge for selectors that don't need it. This should allow us to fix issue #6304 more easily later. And avoid the try {} catch {} penalty.
https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... lib/content/elemHideEmulation.js:155: if (scopeSupported) I think since the code in this if block spans multiple lines we're supposed to add braces. https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... lib/content/elemHideEmulation.js:212: const relativeSelectorRegexp = /^[>]/; Since there's only one character in the group, we could lose the brackets?
https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... lib/content/elemHideEmulation.js:155: if (scopeSupported) On 2018/01/26 08:41:32, Manish Jethani wrote: > I think since the code in this if block spans multiple lines we're supposed to > add braces. Done. https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHid... lib/content/elemHideEmulation.js:212: const relativeSelectorRegexp = /^[>]/; On 2018/01/26 08:41:32, Manish Jethani wrote: > Since there's only one character in the group, we could lose the brackets? Done.
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:138: scopeSupported = false; Maybe we should add that comment here saying that Edge does not support :scope https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:144: * Query selector. If it is relative, will try :scoped. You meant ":scope" (without the D)? https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:147: * @param {bool} all True to perform querySelectorAll() Since all is optional and defaults to false, I guess this should be "[all=false]". Also I think "true" should not be capitalized. https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:148: * @returns {Node|NodeList} result of the query. null in case of error. Since this can return null I think it should be "{?(Node|NodeList)}".
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:214: const relativeSelectorRegexp = /^>/; 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.
I've modified your test in a way it likely to be used and it looks like there
are no significant difference between charCodeAt and direct access.
Here is my code:
{
// prepare data
let u = [];
let n = 10000000;
let r = /^>/;
let c, s, t, func;
for (let i = 0; i < n; i++)
{
if (Math.random() < .1)
u.push(">foo bar");
else
u.push("foo bar");
}
// check regexp
c = 0;
func = s => r.test(s);
s = performance.now();
for (let i = 0; i < n; i++)
if (func(u[i]))
c++;
t = (performance.now() - s) | 0;
console.log(c + " matches found in " + t + "ms with regexp.test()");
// check charCodeAt
c = 0;
func = s => s.charCodeAt(0) == 62;
s = performance.now();
for (let i = 0; i < n; i++)
if (func(u[i]))
c++;
t = (performance.now() - s) | 0;
console.log(c + " matches found in " + t + "ms with str.charCodeAt()");
// check direct access
c = 0;
func = s => s[0] == '>';
s = performance.now();
for (let i = 0; i < n; i++)
if (func(u[i]))
c++;
t = (performance.now() - s) | 0;
console.log(c + " matches found in " + t + "ms with direct access");
// check str.startsWith()
c = 0;
func = s => s.startsWith('>');
s = performance.now();
for (let i = 0; i < n; i++)
if (func(u[i]))
c++;
t = (performance.now() - s) | 0;
console.log(c + " matches found in " + t + "ms with str.startsWith()");
}
Average numbers I got:
Chrome 63:
~300 - regexp
40~50 - charCodeAt (same with direct check in the loop)
90~110 - direct access (same with direct check in the loop)
~660 - startsWith
Firefox 59b5:
~550 - regexp
320~380 - charCodeAt (~460 with direct check in the loop)
325~400 - direct access (~270 with direct check in the loop)
~550 - startsWith
So, in order to get better performance in Fx the whole loop with check should be
implemented separately. At the same time charCodeAt wrapped in arrow function
ends up slightly faster than direct check wrapped in the same way. It seems like
Google Chrome inlines arrow functions and wrapping code in an arrow function
doesn't affect performance in any significant way.
However it's true that regexp is significantly worse in both cases (and
startsWith is even worse than regexp in Chrome).
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:148: * @returns {Node|NodeList} result of the query. null in case of error. On 2018/01/30 05:53:41, Manish Jethani wrote: > Since this can return null I think it should be "{?(Node|NodeList)}". 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.
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:138: scopeSupported = false; On 2018/01/30 05:53:41, Manish Jethani wrote: > Maybe we should add that comment here saying that Edge does not support :scope Done. https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:144: * Query selector. If it is relative, will try :scoped. On 2018/01/30 05:53:41, Manish Jethani wrote: > You meant ":scope" (without the D)? Done. https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:147: * @param {bool} all True to perform querySelectorAll() On 2018/01/30 05:53:41, Manish Jethani wrote: > Since all is optional and defaults to false, I guess this should be > "[all=false]". > > Also I think "true" should not be capitalized. Done. https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:148: * @returns {Node|NodeList} result of the query. null in case of error. On 2018/01/30 05:53:41, Manish Jethani wrote: > Since this can return null I think it should be "{?(Node|NodeList)}". Done. https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHid... lib/content/elemHideEmulation.js:214: const relativeSelectorRegexp = /^>/; On 2018/01/30 06:13:06, Manish Jethani wrote: > 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. Done.
https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid... lib/content/elemHideEmulation.js:214: const incompletePrefixRegexp = /[\s>+~]$/; 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.
LGTM https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid... lib/content/elemHideEmulation.js:214: const incompletePrefixRegexp = /[\s>+~]$/; On 2018/01/30 18:07:28, lainverse wrote: > Since that other regexp were replaced with string comparison it might be a good > idea to replace this one too: > [snip] If we do this then I think that it should be a separate patch. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
