Issue 29383960: Issue 3143 - Filter elements with :-abp-has() (Closed)
|
Created:
March 14, 2017, 11:20 p.m. by hub Modified:
June 14, 2017, 1:16 a.m. CC:
Felix Dahlke Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 3143 - Filter elements with :-abp-has()
Patch Set 1 #Patch Set 2 : Linter pass #Patch Set 3 : Hide elements and not using styles. #Patch Set 4 : Fixed several bugs. Cleanup code. #Patch Set 5 : Support -adb-properties inside :has(). #Patch Set 6 : More cleanup, more robust tests #Patch Set 7 : :has(:has()) do work now. All test pass. #
Total comments: 6
Patch Set 8 : Updated patch following feedback. #Patch Set 9 : Lots of cleanup. Simpler, better. #Patch Set 10 : Updated the filter syntax #
Total comments: 6
Patch Set 11 : Validate the syntax, remove unused flags #
Total comments: 19
Patch Set 12 : Moved unwrap to filterClass, ES6 syntax, use :-abp-has(), don't modify DOM to select, rebased on 29… #Patch Set 13 : Reworked the pseudo class to generate selectors. #Patch Set 14 : Non regexp based parser. #Patch Set 15 : Now we properly upgrade to the new syntax for [-abp-properties=] #Patch Set 16 : Filter reporting was broken in the previous revision. #
Total comments: 2
Patch Set 17 : Added validate of element id, and fixed an infinite recursion in parsing. #
Total comments: 69
Patch Set 18 : Reworked patch following the feedback #Patch Set 19 : Remove unused getElements for PlainSelector and PropsSelector. #
Total comments: 35
Patch Set 20 : Rebased on master. Handled all the feedback. #
Total comments: 12
Patch Set 21 : Addressed the comments. #
Total comments: 8
Patch Set 22 : Address the last comment #Patch Set 23 : Last comments. #
Total comments: 30
Patch Set 24 : Revised patch #Patch Set 25 : One non code change was missing. #
Total comments: 1
Patch Set 26 : Make sure we run without expecting browser environment. #
Total comments: 2
Patch Set 27 : Fix reportError and the error message #
MessagesTotal messages: 52
Felix, This is my initial version of the implementation for feedback. if this way of doing things is ok, there is probably some cleanup, details, and performance issue to solve. Also it still only deal with the sample case. Currently can't have :has() and -abp-properties, this should be solvable. For now, I change the id the the elements selected by :has() if they don't have one in order to select them in CSS, but I'm sure there is a better way. Have added a certain number of tests, some are independent of this feature, and they all pass. But I haven't tested in real life yet. Thanks
This update hide the elements directly instead of using styles. I think it is a better way.
the test that currently fail is because I haven't finished implementing it. I'm more confident with this approach. Note: it change the API for the ElemHideEmulation adding a new callback.
Right now we only miss supporting :has(:has()). The library clients (add-ons) needs to be updated. Comments?
:has(:has()) now work.
https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:13: Element.prototype.mozMatchesSelector || These fallbacks seem unnecessary. We only support Firefox >=38. Chrome >=49 as well as the respective version of Opera, and Microsoft Edge, with this code. Safari and Internet Explorer doesn't need to be considered here at all. https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:32: return sepIndex; There is a lot of duplication above. How about using a loop, or a function, for the repeated logic? Or just a regular expression (assuming that it doesn't have too bad performance implications)? https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:267: var filters = []; // don't need this These are actually needed for the developer tools panel: https://adblockplus.org/development-builds/bringing-blockable-items-to-chrome... Previously the filters where passed as second argument to addSelectorsFunc(), which then if the respective selector matches, where logged into the the developer tools panel. While for filters using the :has() selector, we don't call addSelectorsFunc(), we should make sure to report the applied filters (for the devtools panel) by other means.
will update patch. https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:13: Element.prototype.mozMatchesSelector || On 2017/03/29 07:46:19, Sebastian Noack wrote: > These fallbacks seem unnecessary. We only support Firefox >=38. Chrome >=49 as > well as the respective version of Opera, and Microsoft Edge, with this code. > Safari and Internet Explorer doesn't need to be considered here at all. I need at least the webkit one for PhantomJS, otherwise the test will never pass. This until we fix issue #4796 https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:32: return sepIndex; Done https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elem... chrome/content/elemHideEmulation.js:267: var filters = []; // don't need this On 2017/03/29 07:46:19, Sebastian Noack wrote: > These are actually needed for the developer tools panel: > https://adblockplus.org/development-builds/bringing-blockable-items-to-chrome... > > Previously the filters where passed as second argument to addSelectorsFunc(), > which then if the respective selector matches, where logged into the the > developer tools panel. While for filters using the :has() selector, we don't > call addSelectorsFunc(), we should make sure to report the applied filters (for > the devtools panel) by other means. At that level we might be processing fragments of the filter. Line 356 we'll get the actual filter match that we want for the complete filter.
Actually the implementation has a major problem. If I do ":has(div.aclass)" as expected it will select all the parents. But if do something like ":has(> div.aclass)" it doesn't work. I know why: I didn't read correctly the CSS4 spec.
On 2017/03/31 04:14:57, hub wrote: > Actually the implementation has a major problem. > > If I do ":has(div.aclass)" as expected it will select all the parents. > > But if do something like ":has(> div.aclass)" it doesn't work. I know why: I > didn't read correctly the CSS4 spec. Don't bother reviewing right now, the next update fixes a lot of things. (and make it simpler).
this patch is much better. it works in the chrome addon with my patch.
I only had a pretty high level look, but the approach looks good as far as I can tell. Just two comments. (Note that I'm sceptical about whether we need ABP_ATTR and whether we need to keep addSelectorsFunc, but I'd probably just slow those discussions down. So I'll leave these for Wladimir.) https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); How I understand the code, :has and :-abp-properties would work outside [-abp-selector]. I think that shouldn't be possible - otherwise filter list authors might opt for the more convenient approach of leaving [-abp-selector] out. https://codereview.adblockplus.org/29383960/diff/29408705/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/lib/filterClasses.j... lib/filterClasses.js:1052: this.abpSelectorFilter = (selector.indexOf("[-abp-selector=") != -1); When I first renamed CSS property filters to element hiding emulation filters, I also wanted to introduce flags for what kind of features are going to be emulated. However, we concluded that this is just unused code with no real benefit, see the old discussion here: https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j...
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 10:55:12, Felix Dahlke wrote: > How I understand the code, :has and :-abp-properties would work outside > [-abp-selector]. I think that shouldn't be possible - otherwise filter list > authors might opt for the more convenient approach of leaving [-abp-selector] > out. My question is do we still support [-abp-properties=] after this? OTOH I'll make sure a lonely :has() is rejected. https://codereview.adblockplus.org/29383960/diff/29408705/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/lib/filterClasses.j... lib/filterClasses.js:1052: this.abpSelectorFilter = (selector.indexOf("[-abp-selector=") != -1); On 2017/04/10 10:55:12, Felix Dahlke wrote: > When I first renamed CSS property filters to element hiding emulation filters, I > also wanted to introduce flags for what kind of features are going to be > emulated. However, we concluded that this is just unused code with no real > benefit, see the old discussion here: > https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... They are unused here too. I just shall remove them. I think they are leftover from code that got removed.
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 12:29:27, hub wrote: > On 2017/04/10 10:55:12, Felix Dahlke wrote: > > How I understand the code, :has and :-abp-properties would work outside > > [-abp-selector]. I think that shouldn't be possible - otherwise filter list > > authors might opt for the more convenient approach of leaving [-abp-selector] > > out. > > My question is do we still support [-abp-properties=] after this? > OTOH I'll make sure a lonely :has() is rejected. Lonely :-abp-properties (pseudo class syntax!) should also not work, same as :has. As for [-abp-properties] (attribute syntax): Yeah I think we need to support it at least until the lists we are aware of stopped using it in favour of [-abp-selector].
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elem... chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 12:35:41, Felix Dahlke wrote: > On 2017/04/10 12:29:27, hub wrote: > > On 2017/04/10 10:55:12, Felix Dahlke wrote: > > > How I understand the code, :has and :-abp-properties would work outside > > > [-abp-selector]. I think that shouldn't be possible - otherwise filter list > > > authors might opt for the more convenient approach of leaving > [-abp-selector] > > > out. > > > > My question is do we still support [-abp-properties=] after this? > > OTOH I'll make sure a lonely :has() is rejected. > > Lonely :-abp-properties (pseudo class syntax!) should also not work, same as > :has. > > As for [-abp-properties] (attribute syntax): Yeah I think we need to support it > at least until the lists we are aware of stopped using it in favour of > [-abp-selector]. Ah right. The pseudo class syntax. Good catch.
Updated patch to take felix comments into account. And added tests for it.
I've only done a high-level review of the approach so far. There is rather significant complexity being built up here, largely due to handling of :has() and :-abp-properties() being interdependent. Also, we have different handling of "pure" property selectors and those mixed with :has() which is suboptimal (see comments below). I don't thing that the current approach can scale, particularly if we add more pseudos such as :has-text(). https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:2: // used in the browser tests. See https://issues.adblockplus.org/ticket/4796 This is really a PITA. Given that https://issues.adblockplus.org/ticket/4796 is no longer blocked, I wonder whether we can fix it before changing this script. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:58: e.removeAttribute(ABP_ATTR); This hack is problematic. Even if we make the attribute random, the website can get notified about us adding it via mutation observers and mess with us (e.g. by removing it again immediately). Given that the point is creating a selector to uniquely identify an element, there is a better solution - we can generate a selector of the form :root > :nth-child(2) > :nth-child(8) > :nth-child(1). Given that the browser evaluates CSS selectors starting at the end, this won't even be noticeable slower than what you have here. Note that these selectors won't stay valid if the document structure changes, but they don't have to - as long as the code is evaluated synchronously and we don't touch the DOM the website cannot become active and change something. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:80: function unwrapPattern(pattern) This unwrapping belongs into lib/filterClasses.js IMHO - it needs to recognize emulation filters by the fact that their selector matches /^\[-abp-selector=(["']).*\1\]$/, so it can return the "inner part" as the actual selector. The content script shouldn't get the wrapper at all. Note that IMHO the entire selector should be wrapped in [-abp-selector="..."] because the entire selector will be processed via our custom logic. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:106: match = pseudoClassHasSelectorRegExp.exec(selector); The syntax is getting too complex, parsing CSS with regular expressions won't work. For example, your current regular expression will mistreat "p:has(div) > span:not(.foo)". If you switch to non-greedy expressions you will get issues with "p:has(div:not(.foo))". There is no way around it - we need to count parentheses properly and consider quotation marks and backslashes. Feel free to test which one is faster in JavaScript - going though characters one by one like you would do in C++ or using `selector.replace(/([^()"'\\]+)|([()"']|(\\.))/g, match => {...})`. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:119: match = propertySelectorRegExp.exec(selector); I don't think that we should have this special handling for the legacy property filter syntax here. We can instead modify ElemHideBase.fromText() in lib/filterClasses.js in such a way that it converts such filters immediately - 'example.com##foo [-abp-properties="foobar"] bar' should become 'example.com##[-abp-selector="foo :-abp-properties(foobar) bar"]' and show up in the filter list like that. So when we are working with filters we can assume that they always have the current syntax. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:182: var haveEl = pattern.prefix ? node.querySelectorAll(pattern.prefix) : [ node ]; This will work correctly for selectors like "div > span:has(...)" - but you should also consider the scenario where we have :has() without additional qualifiers, e.g. "div > span > :has(...)". Looking at https://drafts.csswg.org/selectors-3/#selectors, we need special handling for combinator selectors. So if prefix ends with [\s>+~] we need to add "*" to it. For my example you would run node.querySelectorAll("div > span > *") here. There is also the question how we are supposed to deal with selectors like "div:has(...):not(...)". We could detect :has() not being followed by a combinator and reorder it so that we effectively process "div:not(...):has(...)". It should be easier however to mandate that :has() should always be the last specifier before a combinator and merely enforce it here. So if we find :has() that is not at the end of the selector and is not followed by [\s>+~] we should warn about an invalid selector and ignore it. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:259: findPropsSelectors(stylesheets[i], [this.parsed], selectors, filters); We will be going through the stylesheets multiple times, this is rather suboptimal. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:291: this.hideElementsFunc = hideElementsFunc; I guess that hideElementsFunc should do some kind of DOM modification. If somehow possible, we should avoid that, already because we might end up fighting mutation observers then. Ideal solution would generate selectors for the elements to be hidden and pass them to the existing addSelectorsFunc.
Sorry, I published my comments prematurely. I meant to write some more.
First of all, I think that we should have more generic parsing. The parser
should produce a structure like the following:
[
PlainSelector("foo > "),
HasSelector("div.ad"),
PlainSelector(" > span"),
PropsSelector("background-color: #f00")
]
All three classes should provide the same API, so that compiling the results is
a matter of walking this list, calling the same method for all objects.
For example, PlainSelector should probably be implemented like this:
function PlainSelector(selector)
{
this._selector = selector;
}
PlainSelector.prototype = {
*getSelectors(prefix)
{
yield prefix + this._selector;
},
*getElements(prefix)
{
for (let selector of this._getSelectors())
for (let element of document.queryElementsAll(selector)
yield element;
}
};
Implementation of HasSelector() would then be similar but the main function
would be getElements() - it would convert the prefix into elements (adding "*"
to it if necessary as indicated before), then check the condition against all
these elements and only return the ones that matched. Implementation of
getSelectors() on the other hand would take the results of getElements() and
turn them into unique selectors of the form :root > nth-child(...) > ...
PropsSelector() is working selector-based again, it will produce a number of
selectors to be combined with the prefix. We should improve performance here
however: each PropsSelector() instance should add itself to a list when created
by the parser. We should then go through the stylesheets only once, checking
every rule against all PropsSelector() instances. Matching selectors should be
added to the PropsSelector() instance, so when the API methods are called these
selectors are merely retrieved but no stylesheet scanning is necessary any more.
With the current approach (using hideElementsFunc for selectors containing
:has()) the evaluation for chains without HasSelector() instances should work
like this:
function* evaluate(chain, index, prefix)
{
if (index >= chain.length)
return prefix;
for (let selector of chain[index].getSelectors(prefix))
yield* evaluate(chain, index + 1, selector);
}
For chains with a HasSelector() we would call getElements() instead of
getSelectors() in the last step, so we would get a list of elements rather than
a list of selectors.
However, I would like to avoid messing with the DOM altogether as I mentioned
already. The selectors we generate here aren't stable, these will become invalid
as the DOM changes. Question is: can we use mutation observers to detect which
selectors are no longer valid and which subtrees we need to reevaluate?
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem...
chrome/content/elemHideEmulation.js:7: var pseudoClassPropsSelectorRegExp =
/:-abp-properties\((["'])([^"']+)\1\)/;
Having :has() but :-abp-properties() is inconsistent. Either we say that
[-abp-selector="..."] wrapper is sufficient as a warning and we can have our
custom pseudos inside unprefixed. Or we say that we want to indicate our custom
pseudos explicitly and then it should be :-abp-has() as well. I don't think that
we can claim standards compliance and compatibility with future browser
implementations for our :has() pseudo, so it isn't any better off than
:properties(). If you ask me, it's probably better to have :-abp-has() and
:-abp-properties().
I'll rework the patch to implement these suggestions. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:2: // used in the browser tests. See https://issues.adblockplus.org/ticket/4796 On 2017/04/25 10:57:52, Wladimir Palant wrote: > This is really a PITA. Given that https://issues.adblockplus.org/ticket/4796 is > no longer blocked, I wonder whether we can fix it before changing this script. If issue 4796 lands before this, then I'll fix it. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:7: var pseudoClassPropsSelectorRegExp = /:-abp-properties\((["'])([^"']+)\1\)/; On 2017/04/25 11:30:46, Wladimir Palant wrote: > Having :has() but :-abp-properties() is inconsistent. Either we say that > [-abp-selector="..."] wrapper is sufficient as a warning and we can have our > custom pseudos inside unprefixed. Or we say that we want to indicate our custom > pseudos explicitly and then it should be :-abp-has() as well. I don't think that > we can claim standards compliance and compatibility with future browser > implementations for our :has() pseudo, so it isn't any better off than > :properties(). If you ask me, it's probably better to have :-abp-has() and > :-abp-properties(). OK. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:58: e.removeAttribute(ABP_ATTR); On 2017/04/25 10:57:52, Wladimir Palant wrote: > This hack is problematic. Even if we make the attribute random, the website can > get notified about us adding it via mutation observers and mess with us (e.g. by > removing it again immediately). Given that the point is creating a selector to > uniquely identify an element, there is a better solution - we can generate a > selector of the form :root > :nth-child(2) > :nth-child(8) > :nth-child(1). > Given that the browser evaluates CSS selectors starting at the end, this won't > even be noticeable slower than what you have here. Note that these selectors > won't stay valid if the document structure changes, but they don't have to - as > long as the code is evaluated synchronously and we don't touch the DOM the > website cannot become active and change something. I was thinking we could generate an actually random attribute to counter this, but this would just be raising the bar. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:80: function unwrapPattern(pattern) On 2017/04/25 10:57:53, Wladimir Palant wrote: > This unwrapping belongs into lib/filterClasses.js IMHO - it needs to recognize > emulation filters by the fact that their selector matches > /^\[-abp-selector=(["']).*\1\]$/, so it can return the "inner part" as the > actual selector. The content script shouldn't get the wrapper at all. Note that > IMHO the entire selector should be wrapped in [-abp-selector="..."] because the > entire selector will be processed via our custom logic. ok, moving it. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:119: match = propertySelectorRegExp.exec(selector); On 2017/04/25 10:57:52, Wladimir Palant wrote: > I don't think that we should have this special handling for the legacy property > filter syntax here. We can instead modify ElemHideBase.fromText() in > lib/filterClasses.js in such a way that it converts such filters immediately - > 'example.com##foo [-abp-properties="foobar"] bar' should become > 'example.com##[-abp-selector="foo :-abp-properties(foobar) bar"]' and show up in > the filter list like that. So when we are working with filters we can assume > that they always have the current syntax. good idea. Like unwrap above. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:182: var haveEl = pattern.prefix ? node.querySelectorAll(pattern.prefix) : [ node ]; On 2017/04/25 10:57:53, Wladimir Palant wrote: > This will work correctly for selectors like "div > span:has(...)" - but you > should also consider the scenario where we have :has() without additional > qualifiers, e.g. "div > span > :has(...)". Looking at > https://drafts.csswg.org/selectors-3/#selectors, we need special handling for > combinator selectors. So if prefix ends with [\s>+~] we need to add "*" to it. > For my example you would run node.querySelectorAll("div > span > *") here. > > There is also the question how we are supposed to deal with selectors like > "div:has(...):not(...)". We could detect :has() not being followed by a > combinator and reorder it so that we effectively process > "div:not(...):has(...)". It should be easier however to mandate that :has() > should always be the last specifier before a combinator and merely enforce it > here. So if we find :has() that is not at the end of the selector and is not > followed by [\s>+~] we should warn about an invalid selector and ignore it. I naively assumed that since these are filters, we could have a more restricted syntax, at least on the extension. Even though sanitisation of the input isn't performed at the moment.
I realized that I oversimplified with my suggestions above - always running
querySelectorAll() on the entire document is wasteful, after a HasSelector we
know which subtree we are in. So the API should take an additional subtree
parameter, and getSelectors() should be able to return a new subtree:
PlainSelector.prototype = {
*getSelectors(prefix, subtree)
{
yield [prefix + this._selector, subtree];
},
*getElements(prefix, subtree)
{
for (let selector of this._getSelectors())
for (let element of subtree.querySelectorAll(selector)
yield element;
}
};
For PlainSelector and PropsSelector the only change is that the
querySelectorAll() call in getElements() is potentially restricted to a subtree.
HasSelector.getSelectors() on the other hand knows which element the selector
applies to, and it should return this element as the new subtree. The evaluate()
function then changes as follows:
function* evaluate(chain, index, prefix, subtree)
{
if (index >= chain.length)
return prefix;
for (let [selector, element] of chain[index].getSelectors(prefix, subtree))
yield* evaluate(chain, index + 1, selector, element);
}
let selectors = [...evaluate(chain, 0, "", document)];
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem...
chrome/content/elemHideEmulation.js:2: // used in the browser tests. See
https://issues.adblockplus.org/ticket/4796
On 2017/04/25 19:42:47, hub wrote:
> If issue 4796 lands before this, then I'll fix it.
As I suggested on IRC, it would probably be easiest if you work on issue 4796 -
I can answer any questions you might have on that. Otherwise I'll try to find
time, adding more ECMAScript 5 code here doesn't seem to be a good idea to me.
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem...
chrome/content/elemHideEmulation.js:182: var haveEl = pattern.prefix ?
node.querySelectorAll(pattern.prefix) : [ node ];
On 2017/04/25 19:42:47, hub wrote:
> I naively assumed that since these are filters, we could have a more
restricted
> syntax, at least on the extension.
Feel free to suggest a meaningful subset of the syntax that we can parse in a
simpler fashion and enforce. Note that enforcing is important, people will
expect things to work as with any CSS selectors - if they don't, we at least
need to produce a meaningful error message.
On 2017/04/25 11:30:46, Wladimir Palant wrote: > However, I would like to avoid messing with the DOM altogether as I mentioned > already. The selectors we generate here aren't stable, these will become invalid > as the DOM changes. Question is: can we use mutation observers to detect which > selectors are no longer valid and which subtrees we need to reevaluate? Thinking a bit more about this, that should be doable. We remember the selectors we found initially and add a mutation observer. If for example we detect the third child of document.body being added or modified, this invalidates all selectors starting with ":root > :nth-child(2) > :nth-child(N)" where N is 3 or larger. Here, ":root > :nth-child(2)" is the selector of the parent element (document.body being the second child of the document element), and :nth-child(N) means the affected elements and all following siblings which got their position in the document modified. We can remove these selectors from the list and re-run evaluation for all chains containing HasSelector() on document.body.children[N] for N being 3 or larger. What we then need is changing addSelectorsFunc into setSelectorsFunc - rather than always adding selectors, we should be able to replace the selectors list for a document, meaning in particular removing outdated selectors. Then we can get rid of hideSelectorsFunc and getElements() method in PlainSelector & Co. - we will always be hiding via selectors, without touching the DOM. This also allows us to handle element addition and removal as required in the issue. What this won't address are DOM modifications affecting :has() conditions for a different tree. For example, if our rule is "foo:has(span) > div" and we are currently hiding the <div> element then removing the <span> element will not unhide the <div> - we will only re-evaluate this rule if a parent node or a previous sibling of the <foo> element are modified, not its children. I guess if we want to handle this kind of changes, we will need each HasSelector() instance to add its own mutation observer that will cause the chain to be reevaluated if changes in the relevant subtree are detected. I don't think we want to add this kind of logic just now.
On 2017/04/26 05:41:22, Wladimir Palant wrote: > we found initially and add a mutation observer. If for example we detect the > third child of document.body being added or modified, Sorry, I meant to write "added or removed," modification is irrelevant in this context.
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:2: // used in the browser tests. See https://issues.adblockplus.org/ticket/4796 On 2017/04/26 00:03:09, Wladimir Palant wrote: > On 2017/04/25 19:42:47, hub wrote: > > If issue 4796 lands before this, then I'll fix it. > > As I suggested on IRC, it would probably be easiest if you work on issue 4796 - > I can answer any questions you might have on that. Otherwise I'll try to find > time, adding more ECMAScript 5 code here doesn't seem to be a good idea to me. Newer patch is now ES6 based on top of your patch. Consider this done. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elem... chrome/content/elemHideEmulation.js:58: e.removeAttribute(ABP_ATTR); On 2017/04/25 19:42:47, hub wrote: Newer patch removed this and followed your other suggestion.
This version is missing just a couple of things like a non-regexp based parser. But now, following Wladimir advice, we only generate selectors.
I have updated the Chrome implementation it is now simpler. And still work.
And as a side note, it applies on today master with Wladimir changes to have ES6.
https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elem... chrome/content/elemHideEmulation.js:80: if (node && node.id && node.id != "") I need to check that node.id is a valid CSS identifier. I have seen cases in real life where the id start with a number causing querySelector() to fail. I already have a patch for that. Will update review with it.
https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elem... chrome/content/elemHideEmulation.js:162: let suffix = parseSelector(selector.substr(suffixStart)); here, if we had been parsing something matching "-abp-*" then we'd infinite recursion. I do have a patch too.
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:61: // 1 base index like for :nth-child() Nit: Here and below, if you add documentation you might just as well use proper JSDoc syntax. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:64: let parentNode = node ? node.parentNode : null; I guess you should be consistent here: if (!node) return 0; let {parentNode} = node; if (!parentNode) return 0; let {children} = parentNode; if (!children.length) return 0; Note that ParentNode.children cannot return null, so you only have to check the length. Or you can skip the check entirely because the length will be checked in the loop below anyway. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:75: return i + 1; I suggest that you simplify this, no need to declare the variable outside the loop: for (let i = 0; i < children.length; i++) if (children[i] == node) return i + 1; return 0; https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:78: function idValid(id) Nit: how about isValidID() as function name? https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:82: if (id === "") id cannot be an empty string at this point because !"" is true - the previous check will return false already. In fact, !0 is also true, so the check above will reject too much. I guess you actually want: return (id != null && id != "" && !/^(-?[0-9]|--)/.test(id)); But why are you singling out IDs starting with a dash? HTML5 says: "There are no other restrictions on what form an ID can take; in particular, IDs can consist of just digits, start with a digit, start with an underscore, consist of just punctuation, etc." https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:93: let newSelector = "#" + node.id; I can see how this shortcut is tempting. The issue is however: even though the id attribute is supposed to be unique, in practice nothing is stopping the webpage from setting the same id on multiple elements. This selector will then apply to all of them. Feel free to try that out. Unfortunately, we have to go by position all the way up. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:99: if (idx > 0) Usually, this will work because there is only one <html> element on the page. However, the page is free to create more <html> elements and then this selector will no longer be unique. Better: if (node == node.ownerDocument.documentElement) return `:root > ${selector}`; let idx = positionInParent(node); ... Or maybe even: if (!node.parentElement) return `:root > ${selector}`; And then you can reduce the number of checks in positionInParent() because you already know that node is valid and has a parent element. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; Nit: you should remove the trailing whitespace on line 101 and only add it here, if there is a selector part to append. In fact, this should be: if (selector) newSelector += " > " + selector; If we don't have an existing selector part then there is no point in concatenating with it. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:111: function parsePropSelPattern(propertyExpression) This is logic which is only used by the PropSelector class. This function and similar ones should be defined as private methods of that class, so that we have this part of the functionality in one place. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:117: .replace("\\x7B ", "{").replace("\\x7D ", "}"); Nit: You need braces around the if body here. Also, please indent the second line to indicate that this is the previous statement being continued. I think that ESLint will flag both issues, please make sure to run it. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:120: return regexpString; IMHO this should return the actual RegExp rather than expecting the caller to convert the string into a RegExp instance. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:128: let abpSelectorIndex = selector.indexOf(":-abp-"); How about you don't just go looking for anything with ":-abp-" but rather for the specific prefixes we are interested in? This will also eliminate the "parsing error" scenario that is adding unnecessary complexity. let match = /:-abp-(properties|has)\(/.exec(selector); if (!match) return [new PlainSelector(selector)]; let startIndex = match.index + match[0].length; if (match[1] == "properties") ... else if (match[1] == "has") ... else throw new Error("Unexpected, should never get here"); Note how this approach avoids using magic numbers when calculating startIndex. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:174: suffixStart = i + 1; I'm not really happy with the way the contents of the pseudos are being parsed - having two different parsing approaches making different assumptions about contents is rather suboptimal. A generic approach isn't significantly more complicated, something like this: let parens = 1; let quoted = null; let i; for (i = startIndex; i < selector.length; i++) { if (selector[i] == "\\") i++; // skip next character, it's escaped else if (quoted) { if (selector[i] == quoted) quoted = null; } else if (selector[i] == "'" || selector[i] == '"') quoted = selector[i]; else if (selector[i] == "(") parens++; else if (selector[i] == ")") { parens--; if (parens == 0) break; } } if (parens != 0) { console.error(new SyntaxError("Failed parsing Adblock Plus selector " + selector + ", didn't find closing parenthesis")); return null; } let contents = selector.substring(startIndex, i); let suffixStart = i + 1; if (match[1] == "properties") selectors.push(new PropsSelector(contents)); else if (match[1] == "has") selectors.push(new HasSelector(contents)); ... Note that we shouldn't merely ignore invalid selectors, we should always report an error so that filter list authors know they've made a mistake. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:191: function matchStyleProps(style, rule, pattern, selectors, filters) There is something wrong with this function. Please try to describe what it is doing. IMHO that description will necessarily get very lengthy because it doesn't isolate a single piece of functionality (matching) but combines it with the processing of the results. IMHO the matchStyleProps call below has to be replaced with something like: if (pattern.regexp.test(style)) filters.push(...generateHidingSelectors(pattern, rule)); https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:198: let subSelector = subSelectors[i]; This should be a for..of loop. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:218: } We are still iterating through all stylesheets for each rule individually. In particular, we will stringify the style rules for each property rule. This is very inefficient. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:251: * getElements() is a generator function returning elements selected. This should be two proper JSDoc comments on the respective methods. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:264: *getElements(prefix, subtree, stylesheet) getElements() is never being called. On the other hand, there is no code watching for DOM modifications either - so going with selectors only will hide the wrong elements whenever the DOM changes. I guess that this is something you still want to address? https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:266: for (let selector of this.getSelectors(prefix, subtree, stylesheet)) Please use proper destructuring - the result isn't a selector but an array: for (let [selector, _] of this.getSelectors(prefix, subtree, stylesheet)) https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:278: ok() Rename this into valid()? https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:286: yield [prefix + makeSelector(element, ""), subtree]; The prefix should be ignored here - the result of makeSelector() is already the full selector, it shouldn't be concatenated with anything. Also, second element of the array returned should be `element`, here you know which subtree you are in so further refining of the selector should no longer search the entire document. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:291: let elements = subtree.querySelectorAll(prefix ? prefix : "*"); This still won't do the right thing for something like "foo > :-abp-has(...)". As I mentioned before, we need to append "*" to the prefix if it is empty or ends with [\s>+~]. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:295: let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet); Why pass empty string rather than newPrefix + " " here? Won't this do the right thing? https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:317: suffix: "", Why do we still have the suffix here if it is always an empty string? Also, does it still make sense to have this pattern object rather than merely passing a regexp? You don't need to pass the prefix, you could rather add it to the findPropsSelectors() results here - you are looping through them anyway. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:321: findPropsSelectors(stylesheet, selPattern, selectors, filters); findPropsSelectors() should really be a generator yielding selector/filter combinations. We no longer have to deal with arrays to store results here. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:330: for (let subElement of element.querySelectorAll(selector)) Please don't pretend that element is meaningful here - it's always subtree for this class. So it should be: for (let [selector, _] of ...) for (let element of subtree.querySelectorAll(selector)) yield element; In fact, it's exactly the same implementation as for PlainSelector. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:343: Nit: This empty line needs to go (ESLint should warn about that IMHO). https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.j... lib/filterClasses.js:929: const abpSelectorRegExp = /\[-abp-selector=(["'])(.+)\1\]/; We never released this syntax, no point for having backwards compatibility for it. https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.j... lib/filterClasses.js:930: const abpPropertySelectorRegExp = /\[-abp-properties=(["'])([^"']+)\1\]/; I definitely think that there should be something about "legacy" in the name. Maybe legacyPropertyRuleRegExp? https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.j... lib/filterClasses.js:994: { This *definitely* needs to go after the isException check - an exception is never an emulation rule. https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.j... lib/filterClasses.js:1000: return new InvalidFilter(text, "filter_elemhideemulation_nodomain"); No need to duplicate code, we have that check done below. https://codereview.adblockplus.org/29383960/diff/29438559/lib/filterClasses.j... lib/filterClasses.js:1022: if (isHideEmulation) How does that support :-abp-properties() and :-abp-has() syntax? This is supposed to be the proper syntax, with [-abp-properties="..."] only supported for backwards compatibility. IMHO the logic should be something like: let legacyMatch = legacyPropertyRuleRegExp.exec(selector); if (legacyMatch) { // Convert legacy CSS Property Rules syntax selector = selector.substr(0, legacyMatch.index) + ":-abp-properties(" + legacyMatch[2] + ")" + selector.substr(legacyMatch.index + legacyMatch[0].length); text = domain + "##" + selector; } if (selector.includes(":-abp-properties(") || selector.includes(":-abp-has(")) { ... } Note that quotation marks shouldn't be kept when converting [-abp-properties="..."], pseudo-classes don't need those.
I'll update the patch with what's left. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:64: let parentNode = node ? node.parentNode : null; On 2017/05/16 13:50:34, Wladimir Palant wrote: > I guess you should be consistent here: > > if (!node) > return 0; > let {parentNode} = node; > if (!parentNode) > return 0; > let {children} = parentNode; > if (!children.length) > return 0; > > Note that ParentNode.children cannot return null, so you only have to check the > length. Or you can skip the check entirely because the length will be checked in > the loop below anyway. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:75: return i + 1; On 2017/05/16 13:50:37, Wladimir Palant wrote: > I suggest that you simplify this, no need to declare the variable outside the > loop: > > for (let i = 0; i < children.length; i++) > if (children[i] == node) > return i + 1; > return 0; Re-reading the code, it almost make it correct. If there was no children it would have returned 1.... And length + 1 of not found.... https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:78: function idValid(id) On 2017/05/16 13:50:34, Wladimir Palant wrote: > Nit: how about isValidID() as function name? Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:82: if (id === "") On 2017/05/16 13:50:33, Wladimir Palant wrote: > id cannot be an empty string at this point because !"" is true - the previous > check will return false already. In fact, !0 is also true, so the check above > will reject too much. I guess you actually want: > > return (id != null && id != "" && !/^(-?[0-9]|--)/.test(id)); > > But why are you singling out IDs starting with a dash? HTML5 says: "There are no > other restrictions on what form an ID can take; in particular, IDs can consist > of just digits, start with a digit, start with an underscore, consist of just > punctuation, etc." https://www.w3.org/TR/CSS2/syndata.html#value-def-identifier https://www.w3.org/TR/CSS2/syndata.html#tokenization Can start with a '-' but not '--', nor '-' followed by a digit. This is CSS not HTML5. It the id valid in a selector. Without that check if I try to select using an ID that, for example, starts with a digit, querySelector() will throw a DOM exception. All in all that thing is probably totally moot given the comment below. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:93: let newSelector = "#" + node.id; On 2017/05/16 13:50:33, Wladimir Palant wrote: > I can see how this shortcut is tempting. The issue is however: even though the > id attribute is supposed to be unique, in practice nothing is stopping the > webpage from setting the same id on multiple elements. This selector will then > apply to all of them. Feel free to try that out. > > Unfortunately, we have to go by position all the way up. :-/ This is really unfortunate. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/05/16 13:50:38, Wladimir Palant wrote: > Nit: you should remove the trailing whitespace on line 101 and only add it here, > if there is a selector part to append. In fact, this should be: > > if (selector) > newSelector += " > " + selector; > > If we don't have an existing selector part then there is no point in > concatenating with it. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:111: function parsePropSelPattern(propertyExpression) On 2017/05/16 13:50:34, Wladimir Palant wrote: > This is logic which is only used by the PropSelector class. This function and > similar ones should be defined as private methods of that class, so that we have > this part of the functionality in one place. I'll move it to the PropsSelector constructor. That's the only place it is called. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:117: .replace("\\x7B ", "{").replace("\\x7D ", "}"); On 2017/05/16 13:50:39, Wladimir Palant wrote: > Nit: You need braces around the if body here. Also, please indent the second > line to indicate that this is the previous statement being continued. I think > that ESLint will flag both issues, please make sure to run it. ESLint doesn't flag this. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:120: return regexpString; On 2017/05/16 13:50:36, Wladimir Palant wrote: > IMHO this should return the actual RegExp rather than expecting the caller to > convert the string into a RegExp instance. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:191: function matchStyleProps(style, rule, pattern, selectors, filters) On 2017/05/16 13:50:36, Wladimir Palant wrote: > There is something wrong with this function. Please try to describe what it is > doing. IMHO that description will necessarily get very lengthy because it > doesn't isolate a single piece of functionality (matching) but combines it with > the processing of the results. > > IMHO the matchStyleProps call below has to be replaced with something like: > > if (pattern.regexp.test(style)) > filters.push(...generateHidingSelectors(pattern, rule)); I did merge that function into findPropsSelectors() because it is only called from there, because to where it originally was. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:198: let subSelector = subSelectors[i]; On 2017/05/16 13:50:37, Wladimir Palant wrote: > This should be a for..of loop. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:251: * getElements() is a generator function returning elements selected. On 2017/05/16 13:50:39, Wladimir Palant wrote: > This should be two proper JSDoc comments on the respective methods. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:266: for (let selector of this.getSelectors(prefix, subtree, stylesheet)) On 2017/05/16 13:50:36, Wladimir Palant wrote: > Please use proper destructuring - the result isn't a selector but an array: > > for (let [selector, _] of this.getSelectors(prefix, subtree, stylesheet)) It would be `let [selector] of `. The `, _` part create an unused variable `_` and eslint complains about it. Also fixed the line below too. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:278: ok() On 2017/05/16 13:50:38, Wladimir Palant wrote: > Rename this into valid()? Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:286: yield [prefix + makeSelector(element, ""), subtree]; On 2017/05/16 13:50:39, Wladimir Palant wrote: > The prefix should be ignored here - the result of makeSelector() is already the > full selector, it shouldn't be concatenated with anything. Also, second element > of the array returned should be `element`, here you know which subtree you are > in so further refining of the selector should no longer search the entire > document. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:291: let elements = subtree.querySelectorAll(prefix ? prefix : "*"); On 2017/05/16 13:50:34, Wladimir Palant wrote: > This still won't do the right thing for something like "foo > :-abp-has(...)". > As I mentioned before, we need to append "*" to the prefix if it is empty or > ends with [\s>+~]. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:295: let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet); On 2017/05/16 13:50:34, Wladimir Palant wrote: > Why pass empty string rather than newPrefix + " " here? Won't this do the right > thing? I pass `element` as the subtree so from here I don't need the pefix for evaluate. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:317: suffix: "", On 2017/05/16 13:50:36, Wladimir Palant wrote: > Why do we still have the suffix here if it is always an empty string? Also, does > it still make sense to have this pattern object rather than merely passing a > regexp? You don't need to pass the prefix, you could rather add it to the > findPropsSelectors() results here - you are looping through them anyway. I think `suffix` left was an oversight from when I reworked the code. Removing it as well as changing the code that still use it. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:321: findPropsSelectors(stylesheet, selPattern, selectors, filters); On 2017/05/16 13:50:37, Wladimir Palant wrote: > findPropsSelectors() should really be a generator yielding selector/filter > combinations. We no longer have to deal with arrays to store results here. we don't even need filter at the point. they are totally unused in that context. Turned into a generator. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:330: for (let subElement of element.querySelectorAll(selector)) On 2017/05/16 13:50:38, Wladimir Palant wrote: > Please don't pretend that element is meaningful here - it's always subtree for > this class. So it should be: > > for (let [selector, _] of ...) > for (let element of subtree.querySelectorAll(selector)) > yield element; > > In fact, it's exactly the same implementation as for PlainSelector. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:343: On 2017/05/16 13:50:35, Wladimir Palant wrote: > Nit: This empty line needs to go (ESLint should warn about that IMHO). Sadly it doesn't.
Just a few things still not resolved. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:61: // 1 base index like for :nth-child() On 2017/05/16 13:50:38, Wladimir Palant wrote: > Nit: Here and below, if you add documentation you might just as well use proper > JSDoc syntax. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:99: if (idx > 0) On 2017/05/16 13:50:33, Wladimir Palant wrote: > Usually, this will work because there is only one <html> element on the page. > However, the page is free to create more <html> elements and then this selector > will no longer be unique. Better: > > if (node == node.ownerDocument.documentElement) > return `:root > ${selector}`; > > let idx = positionInParent(node); > ... > > Or maybe even: > > if (!node.parentElement) > return `:root > ${selector}`; > > And then you can reduce the number of checks in positionInParent() because you > already know that node is valid and has a parent element. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:128: let abpSelectorIndex = selector.indexOf(":-abp-"); On 2017/05/16 13:50:36, Wladimir Palant wrote: > How about you don't just go looking for anything with ":-abp-" but rather for > the specific prefixes we are interested in? This will also eliminate the > "parsing error" scenario that is adding unnecessary complexity. > > let match = /:-abp-(properties|has)\(/.exec(selector); > if (!match) > return [new PlainSelector(selector)]; > > let startIndex = match.index + match[0].length; > if (match[1] == "properties") > ... > else if (match[1] == "has") > ... > else > throw new Error("Unexpected, should never get here"); > > Note how this approach avoids using magic numbers when calculating startIndex. I think I took the "don't use regexp" from the previous review too literally. Will rework this accordingly. I agree, magic numbers are bad. Done https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:174: suffixStart = i + 1; On 2017/05/16 13:50:33, Wladimir Palant wrote: > I'm not really happy with the way the contents of the pseudos are being parsed - > having two different parsing approaches making different assumptions about > contents is rather suboptimal. A generic approach isn't significantly more > complicated, something like this: > > let parens = 1; > let quoted = null; > let i; > for (i = startIndex; i < selector.length; i++) > { > if (selector[i] == "\\") > i++; // skip next character, it's escaped > else if (quoted) > { > if (selector[i] == quoted) > quoted = null; > } > else if (selector[i] == "'" || selector[i] == '"') > quoted = selector[i]; > else if (selector[i] == "(") > parens++; > else if (selector[i] == ")") > { > parens--; > if (parens == 0) > break; > } > } > if (parens != 0) > { > console.error(new SyntaxError("Failed parsing Adblock Plus selector " + > selector + ", didn't find closing parenthesis")); > return null; > } > let contents = selector.substring(startIndex, i); > let suffixStart = i + 1; > if (match[1] == "properties") > selectors.push(new PropsSelector(contents)); > else if (match[1] == "has") > selectors.push(new HasSelector(contents)); > ... > > Note that we shouldn't merely ignore invalid selectors, we should always report > an error so that filter list authors know they've made a mistake. They are parsed differently because -abp-properties() content is simply quoted, ie we ignore everything between the quote. On the other hand, :-abp-has() is unquoted becauase it is a selector. Also current parsing doesn't handle escapes for the quotes. Reworked to a single parser. As for the error reporting, I was not sure what's the best way to do it. I have added a console.error() like you suggested in the right places.
I need to address that part. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:264: *getElements(prefix, subtree, stylesheet) On 2017/05/16 13:50:37, Wladimir Palant wrote: > getElements() is never being called. On the other hand, there is no code > watching for DOM modifications either - so going with selectors only will hide > the wrong elements whenever the DOM changes. I guess that this is something you > still want to address? This is something that I still need to address.
This is the status of the latest revision of the patch. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:218: } On 2017/05/16 13:50:37, Wladimir Palant wrote: > We are still iterating through all stylesheets for each rule individually. In > particular, we will stringify the style rules for each property rule. This is > very inefficient. Done. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:264: *getElements(prefix, subtree, stylesheet) On 2017/05/26 12:42:46, hub wrote: > On 2017/05/16 13:50:37, Wladimir Palant wrote: > > getElements() is never being called. On the other hand, there is no code > > watching for DOM modifications either - so going with selectors only will hide > > the wrong elements whenever the DOM changes. I guess that this is something > you > > still want to address? > > This is something that I still need to address. I removed the unused getElements. But I don't address the DOM changes yet. I'll do that in either: 1. a new revision of this patch 2. another patch with a new review request (possibly holding this checkin back) Whichever is easiest to review.
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/05/16 21:35:53, hub wrote: > > If we don't have an existing selector part then there is no point in > > concatenating with it. > > Acknowledged. Acknowledged but the code is unchanged :) Well, I'm not going to insist that you address a nit. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:264: *getElements(prefix, subtree, stylesheet) On 2017/05/31 02:16:49, hub wrote: > I removed the unused getElements. But I don't address the DOM changes yet. > > I'll do that in either: > 1. a new revision of this patch > 2. another patch with a new review request (possibly holding this checkin back) > Whichever is easiest to review. We (Felix, Sebastian and me) discussed this in the office today. The consensus is that we should go with the solution that is faster to implement https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:295: let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet); On 2017/05/16 21:35:55, hub wrote: > I pass `element` as the subtree so from here I don't need the pefix for > evaluate. But you need it so that you get the right selectors back. In particular, if you have nested :has() selectors, concatenating prefix and the generated selector will produce wrong results (generated selector is rooted). Mind you, querySelectorAll() might be somewhat slower because you are passing in a longer selector - this shouldn't be significant however. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:61: * @param {Node} node - the node to find the position of. Nit (here and elsewhere): the minus sign is unnecessary, the format doesn't expect any separators between parameter name and description. I assume that JSDoc will make it part of the description which is ugly. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:62: * @return {number} 1 base index like for :nth-child(), or 0 on error. Nit: I think it's "One-based" https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:67: return 0; Nit: With the current code we can no longer get null for node here. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:96: const abpSelectorRegexp = /:-abp-(properties|has|[A-Za-z\d-]*)\(/i; Ok, let's say that [A-Za-z\d-]* clause is forward compatibility - new pseudos introduced in future should cause the filter to be ignored. But I would prefer [\w-]+ here which is simpler and makes sure that there is at least one character. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:98: function parseSelectorContent(content, quoted = false) Why this quoted parameter? As I mentioned before, -abp-properties(...) contents aren't supposed to be quoted - they only were before because it was an "attribute" but now we have a pseudo-class. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:105: content = content.trim(); This call will remove whitespace on both sides - you will get bogus results if whitespace is removed on the left side, because suffixStart calculation below will be incorrect. Either way, if we want to remove the whitespace we should do it in PropsSelector constructor - not sure whether we do, IMHO the original code didn't. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:106: while (i < content.length) This should really be a for loop: for (let i = 0; i < content.length; i++) https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:158: selectors.push(new PlainSelector(selector.substr(0, suffixStart))); I don't think that reusing suffixStart variable for two very different values helps make this code readable. Not that it is really necessary: if (match.index > 0) selectors.push(new PlainSelector(selector.substr(0, match.index))); https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:171: } Setting and checking content variable should be done outside the if block - this logic is common to any pseudo, no need to duplicate code. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:181: "nested :-abp-has().")); While nested :-abp-has() might not be very efficient, does the current code really have issues with it? https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:208: suffixStart = startIndex + content.end + 1; Really, content.end should just have the proper value. So parseSelectorContent() should get the full selector an not a substring, and a startIndex parameter which is the initial value for i. And content.end should be set to the index of the first character after the content. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:219: function *findPropsSelectors(styles, prefix, regexp) This is functionality that is only used by PropsSelector - it should be a member of that class. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:265: * @param {Array} styles - the stringified stylesheets. Nit: The type here should be more specific: {string[]} https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:273: const prefixEndRegexp = /[\s>+~]$/; This variable name doesn't really tell much about the purpose, incompletePrefixRegexp maybe? https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:296: * @param {Array} styles - the stringified stylesheets. Nit: Like above, the type is string[] https://codereview.adblockplus.org/29383960/diff/29452284/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/lib/filterClasses.j... lib/filterClasses.js:1: /* None of my comments in this file have been addressed. Note also that the approach has to change now: we decided that a new filter syntax is more appropriate for element hiding emulation filters. Also, I realized that the "translation" of the legacy syntax into the new one has to happen in Filter.fromText() - this function caches filters, so we need to do it there in order to prevent bad caching (two instances of the same filter, once cached as example.com##[-abp-properties="foobar"] and the other time as example.com#?#:-abp-properties(foobar)).
Also re-tested in chrome. The chrome patch will need to be updated. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/01 10:16:32, Wladimir Palant wrote: > On 2017/05/16 21:35:53, hub wrote: > > > If we don't have an existing selector part then there is no point in > > > concatenating with it. > > > > Acknowledged. > > Acknowledged but the code is unchanged :) > > Well, I'm not going to insist that you address a nit. it is done patch set 19, definitely. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:295: let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet); On 2017/06/01 10:16:33, Wladimir Palant wrote: > On 2017/05/16 21:35:55, hub wrote: > > I pass `element` as the subtree so from here I don't need the pefix for > > evaluate. > > But you need it so that you get the right selectors back. In particular, if you > have nested :has() selectors, concatenating prefix and the generated selector > will produce wrong results (generated selector is rooted). Mind you, > querySelectorAll() might be somewhat slower because you are passing in a longer > selector - this shouldn't be significant however. Then below, line 299 I just pass "selector". Done https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:61: * @param {Node} node - the node to find the position of. On 2017/06/01 10:16:36, Wladimir Palant wrote: > Nit (here and elsewhere): the minus sign is unnecessary, the format doesn't > expect any separators between parameter name and description. I assume that > JSDoc will make it part of the description which is ugly. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:62: * @return {number} 1 base index like for :nth-child(), or 0 on error. On 2017/06/01 10:16:38, Wladimir Palant wrote: > Nit: I think it's "One-based" Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:67: return 0; On 2017/06/01 10:16:37, Wladimir Palant wrote: > Nit: With the current code we can no longer get null for node here. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:96: const abpSelectorRegexp = /:-abp-(properties|has|[A-Za-z\d-]*)\(/i; On 2017/06/01 10:16:36, Wladimir Palant wrote: > Ok, let's say that [A-Za-z\d-]* clause is forward compatibility - new pseudos > introduced in future should cause the filter to be ignored. But I would prefer > [\w-]+ here which is simpler and makes sure that there is at least one > character. this regexp is now gone. I use the one from issue 5287. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:98: function parseSelectorContent(content, quoted = false) On 2017/06/01 10:16:35, Wladimir Palant wrote: > Why this quoted parameter? As I mentioned before, -abp-properties(...) contents > aren't supposed to be quoted - they only were before because it was an > "attribute" but now we have a pseudo-class. this is gone too. I may have missed the comment about properties not being quoted. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:105: content = content.trim(); On 2017/06/01 10:16:35, Wladimir Palant wrote: > This call will remove whitespace on both sides - you will get bogus results if > whitespace is removed on the left side, because suffixStart calculation below > will be incorrect. Either way, if we want to remove the whitespace we should do > it in PropsSelector constructor - not sure whether we do, IMHO the original code > didn't. it is gone. see above. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:106: while (i < content.length) On 2017/06/01 10:16:35, Wladimir Palant wrote: > This should really be a for loop: > > for (let i = 0; i < content.length; i++) current version is your parser from issue 5287. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:158: selectors.push(new PlainSelector(selector.substr(0, suffixStart))); On 2017/06/01 10:16:34, Wladimir Palant wrote: > I don't think that reusing suffixStart variable for two very different values > helps make this code readable. Not that it is really necessary: > > if (match.index > 0) > selectors.push(new PlainSelector(selector.substr(0, match.index))); Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:171: } On 2017/06/01 10:16:35, Wladimir Palant wrote: > Setting and checking content variable should be done outside the if block - this > logic is common to any pseudo, no need to duplicate code. there was the exception for "quoted", but now that it is gone, will fix that. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:181: "nested :-abp-has().")); On 2017/06/01 10:16:36, Wladimir Palant wrote: > While nested :-abp-has() might not be very efficient, does the current code > really have issues with it? There was an issue, but it is fixed now. Removing this limitation as well. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:208: suffixStart = startIndex + content.end + 1; On 2017/06/01 10:16:36, Wladimir Palant wrote: > Really, content.end should just have the proper value. So parseSelectorContent() > should get the full selector an not a substring, and a startIndex parameter > which is the initial value for i. And content.end should be set to the index of > the first character after the content. I preferred to just pass a string to parseSelectorContent. but ok. I'll do it that way. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:219: function *findPropsSelectors(styles, prefix, regexp) On 2017/06/01 10:16:37, Wladimir Palant wrote: > This is functionality that is only used by PropsSelector - it should be a member > of that class. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:265: * @param {Array} styles - the stringified stylesheets. On 2017/06/01 10:16:38, Wladimir Palant wrote: > Nit: The type here should be more specific: {string[]} They are not strings. I'll rephrase to say "the stringified stylesheet objects" https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:273: const prefixEndRegexp = /[\s>+~]$/; On 2017/06/01 10:16:34, Wladimir Palant wrote: > This variable name doesn't really tell much about the purpose, > incompletePrefixRegexp maybe? Acknowledged. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:296: * @param {Array} styles - the stringified stylesheets. On 2017/06/01 10:16:37, Wladimir Palant wrote: > Nit: Like above, the type is string[] a above. not a string array.
https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:208: suffixStart = startIndex + content.end + 1; On 2017/06/01 18:22:55, hub wrote: > On 2017/06/01 10:16:36, Wladimir Palant wrote: > > Really, content.end should just have the proper value. So > parseSelectorContent() > > should get the full selector an not a substring, and a startIndex parameter > > which is the initial value for i. And content.end should be set to the index > of > > the first character after the content. > > I preferred to just pass a string to parseSelectorContent. but ok. I'll do it > that way. Well, you didn't. Whatever, not that important I guess... https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:265: * @param {Array} styles - the stringified stylesheets. On 2017/06/01 18:23:00, hub wrote: > On 2017/06/01 10:16:38, Wladimir Palant wrote: > > Nit: The type here should be more specific: {string[]} > > They are not strings. I'll rephrase to say "the stringified stylesheet objects" Right, they aren't, my original assumptions were wrong. How about you use @typedef then: http://usejsdoc.org/tags-typedef.html. This way you should be able to define a StringifiedStyle type properly and use it here. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:138: function parseSelector(selector, level = 0) Level parameter is no longer used. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:151: selectors.push(new PlainSelector(selector.substr(0, match.index))); I meant - suffixStart variable shouldn't be declared/assigned here at all. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:188: hide |= suffix.hide; Using numerical operators on boolean values isn't a good idea in JavaScript, this should really be: hide = hide || suffix.hide; Either way, dragging this state all the way through introduces significant complexity which IMHO isn't worth it. How about you return selectors as you did before and when you need to decide whether a particular chain should hide you do the following: let hide = selectors.some(s => s instanceof HasSelector); If I see it correctly, this check needs to be performed only once per chain, so not worth optimizing. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:344: let filterElements = []; This array contains filters, not elements. How about naming these four variables selectors, selectorFilters, elements and elementFilters? This seems consistent. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:379: for (let element of document.querySelectorAll(selector)) Nit: we require brackets around multi-line blocks, even if these only contain a single statement.
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/01 18:22:54, hub wrote: > On 2017/06/01 10:16:32, Wladimir Palant wrote: > > On 2017/05/16 21:35:53, hub wrote: > > > > If we don't have an existing selector part then there is no point in > > > > concatenating with it. > > > > > > Acknowledged. > > > > Acknowledged but the code is unchanged :) > > > > Well, I'm not going to insist that you address a nit. > > it is done patch set 19, definitely. The trailing whitespace part - yes. Only doing the concatenation if we have a non-empty selector to concatenate with - not so much.
Updated patch https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 08:35:19, Wladimir Palant wrote: > On 2017/06/01 18:22:54, hub wrote: > > On 2017/06/01 10:16:32, Wladimir Palant wrote: > > > On 2017/05/16 21:35:53, hub wrote: > > > > > If we don't have an existing selector part then there is no point in > > > > > concatenating with it. > > > > > > > > Acknowledged. > > > > > > Acknowledged but the code is unchanged :) > > > > > > Well, I'm not going to insist that you address a nit. > > > > it is done patch set 19, definitely. > > The trailing whitespace part - yes. Only doing the concatenation if we have a > non-empty selector to concatenate with - not so much. Current code is: let newSelector = `${node.tagName}:nth-child(${idx})`; if (selector) newSelector += " > "; Maybe I'm missing the obvious, but I only concatenate if we have a non empty selector, don't I? https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:208: suffixStart = startIndex + content.end + 1; On 2017/06/07 08:32:57, Wladimir Palant wrote: > On 2017/06/01 18:22:55, hub wrote: > > On 2017/06/01 10:16:36, Wladimir Palant wrote: > > > Really, content.end should just have the proper value. So > > parseSelectorContent() > > > should get the full selector an not a substring, and a startIndex parameter > > > which is the initial value for i. And content.end should be set to the index > > of > > > the first character after the content. > > > > I preferred to just pass a string to parseSelectorContent. but ok. I'll do it > > that way. > > Well, you didn't. Whatever, not that important I guess... I did really intend to do it. Done now. https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elem... chrome/content/elemHideEmulation.js:265: * @param {Array} styles - the stringified stylesheets. On 2017/06/07 08:32:57, Wladimir Palant wrote: > On 2017/06/01 18:23:00, hub wrote: > > On 2017/06/01 10:16:38, Wladimir Palant wrote: > > > Nit: The type here should be more specific: {string[]} > > > > They are not strings. I'll rephrase to say "the stringified stylesheet > objects" > > Right, they aren't, my original assumptions were wrong. How about you use > @typedef then: http://usejsdoc.org/tags-typedef.html. This way you should be > able to define a StringifiedStyle type properly and use it here. Done. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:138: function parseSelector(selector, level = 0) On 2017/06/07 08:32:58, Wladimir Palant wrote: > Level parameter is no longer used. I meant to remove it. it is gone now. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:151: selectors.push(new PlainSelector(selector.substr(0, match.index))); On 2017/06/07 08:32:58, Wladimir Palant wrote: > I meant - suffixStart variable shouldn't be declared/assigned here at all. done https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:188: hide |= suffix.hide; On 2017/06/07 08:32:58, Wladimir Palant wrote: > Using numerical operators on boolean values isn't a good idea in JavaScript, > this should really be: > > hide = hide || suffix.hide; > > Either way, dragging this state all the way through introduces significant > complexity which IMHO isn't worth it. How about you return selectors as you did > before and when you need to decide whether a particular chain should hide you do > the following: > > let hide = selectors.some(s => s instanceof HasSelector); > > If I see it correctly, this check needs to be performed only once per chain, so > not worth optimizing. The problem with moving this out is that the decision of hiding is performed somewhere else and not in the parse selector logic. When I implemented :-abp-contains (it is another patch), I kept the changes in here. This is a different tradeoff I guess, but I prefer it that way. I have changed this line though, to do as you suggested. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:344: let filterElements = []; On 2017/06/07 08:32:59, Wladimir Palant wrote: > This array contains filters, not elements. How about naming these four variables > selectors, selectorFilters, elements and elementFilters? This seems consistent. make sense. done. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:379: for (let element of document.querySelectorAll(selector)) On 2017/06/07 08:32:59, Wladimir Palant wrote: > Nit: we require brackets around multi-line blocks, even if these only contain a > single statement. Done.
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 14:15:06, hub wrote: > Current code is: > > let newSelector = `${node.tagName}:nth-child(${idx})`; > if (selector) > newSelector += " > "; > > Maybe I'm missing the obvious, but I only concatenate if we have a non empty > selector, don't I? It's about the concatenation on the next line ;) let newSelector = `${node.tagName}:nth-child(${idx})`; if (selector) newSelector += " > " + selector; return makeSelector(node.parentNode, newSelector); https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:188: hide |= suffix.hide; On 2017/06/07 14:15:09, hub wrote: > The problem with moving this out is that the decision of hiding is performed > somewhere else and not in the parse selector logic. Assuming that I understood this sentence correctly, you can still generalize this by adding a requiresHiding property to the prototype of HasSelector and such - it would be true for HasSelector and false for everything else. You could still replace this whole lot of complexity by four lines of code where three are trivial. And you replace `if (!pattern.hide)` (the only place where this property is actually used) by: if (!selectors.some(s => s.requiresHiding)) https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:128: return {text: content.substr(startIndex, i - startIndex), end: i}; content.substring(startIndex, i) will do. https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:227: * @param {...StringifiedStyle} styles the stringified style objects. No, ...StringifiedStyle would mean there is a variable number of parameters, each typed StringifiedStyle. That's not what you have here, it's StringifiedStyle[]. https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:360: /* Stringified style objects This should be /** (with two stars). https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:364: */ It would make sense to have this type declared before stringifyStyle() function definition, not where it is called. Of course, it would also make sense to add a JSDoc comment for the function itself then.
Patch updated. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elem... chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 14:53:40, Wladimir Palant wrote: > On 2017/06/07 14:15:06, hub wrote: > > Current code is: > > > > let newSelector = `${node.tagName}:nth-child(${idx})`; > > if (selector) > > newSelector += " > "; > > > > Maybe I'm missing the obvious, but I only concatenate if we have a non empty > > selector, don't I? > > It's about the concatenation on the next line ;) > > let newSelector = `${node.tagName}:nth-child(${idx})`; > if (selector) > newSelector += " > " + selector; > return makeSelector(node.parentNode, newSelector); Ah !. But if selector is empty, it doesn't matter, it is like a no-op. I'll amend my patch. Hear and above.
I hope that you didn't overlook the other four comments. I'll wait for those to be addressed.
You were right, I mistakenly overlooked this other comment (I totally skipped them). Now all addressed. https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29453712/chrome/content/elem... chrome/content/elemHideEmulation.js:188: hide |= suffix.hide; On 2017/06/07 14:53:41, Wladimir Palant wrote: > On 2017/06/07 14:15:09, hub wrote: > > The problem with moving this out is that the decision of hiding is performed > > somewhere else and not in the parse selector logic. > > Assuming that I understood this sentence correctly, you can still generalize > this by adding a requiresHiding property to the prototype of HasSelector and > such - it would be true for HasSelector and false for everything else. You could > still replace this whole lot of complexity by four lines of code where three are > trivial. And you replace `if (!pattern.hide)` (the only place where this > property is actually used) by: > > if (!selectors.some(s => s.requiresHiding)) done https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:128: return {text: content.substr(startIndex, i - startIndex), end: i}; On 2017/06/07 14:53:42, Wladimir Palant wrote: > content.substring(startIndex, i) will do. ok https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:227: * @param {...StringifiedStyle} styles the stringified style objects. On 2017/06/07 14:53:42, Wladimir Palant wrote: > No, ...StringifiedStyle would mean there is a variable number of parameters, > each typed StringifiedStyle. That's not what you have here, it's > StringifiedStyle[]. Done. https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:360: /* Stringified style objects On 2017/06/07 14:53:42, Wladimir Palant wrote: > This should be /** (with two stars). Done. https://codereview.adblockplus.org/29383960/diff/29458616/chrome/content/elem... chrome/content/elemHideEmulation.js:364: */ On 2017/06/07 14:53:43, Wladimir Palant wrote: > It would make sense to have this type declared before stringifyStyle() function > definition, not where it is called. Of course, it would also make sense to add a > JSDoc comment for the function itself then. stringifyStyle return a string. This is where we build that "stringified style" object. Maybe I will just move that to stringifyStyle. And the documentation.
https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:152: if (content == null) Nit: Given that parseSelectorContent() either returns and object or null, perhaps `!content` is more appropriate here rather than `content == null`. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing AdBlock Plus " + Nit: It's Adblock Plus (with lower case "B"). But perhaps we should avoid mentioning the product name in error messages in the first place, in order to avoid confusion in other products that are based on this code. (Same below.) https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:205: styles.push(property + ": " + value + (priority ? " !" + priority : "") + Nit: If you'd use a template literal here you woudnl't have to wrap this line: `${property}: ${value}${priority ? " !" + priority : ""};`
I didn't finish going through the unit tests today, but I thought that I would publish my existing comments already. The actual change is ok now, only nits remaining. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:188: * @property {string} style CSS style represented a string. This doesn't seem to be quite English to me, also not very helpful. "sorted and joined list of CSS properties" maybe? Note that this isn't a full sentence, so there shouldn't be a period at the end. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:189: * @property {string[]} subSelectors selectors for the rule. "selectors that the CSS properties apply to"? https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:193: * Turn a CSSStyleRule into a StringifiedStyle You have the types listed below, no need to repeat them. "Produces a string representation of a stylesheet entry." maybe? Note that this is a sentence, so it should end with a period. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:35: document.body.removeChild(child); If you store the child in an intermediate variable, you should use it: child.parentNode.removeChild(child); https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:80: // insert a <div> with a unique id and and empty CSS rule You have "and" twice here. Also, how is this an empty CSS rule? It actually has a value, according to the styleBlock parameter. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:95: // and then will call the callback. There are no callbacks here, this function returns a promise. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:97: // loop There is no loop here. But - yes, loadScript() might not detect the error condition and reject the promise. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:114: // instantiate a ElemHideEmulation with @selectors. "an ElemHideEmulation instance"? https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:140: return; This check is pointless, we can just let the loop run even with an empty array. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:147: return Promise.resolve(); While it was like that before your changes already, it would probably make sense to return Promise.resolve(elemHideEmulation) here. This way you can test ElemHideEmulation properties where other options don't work. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:169: }).catch(unexpectedError.bind(test)).then(() => test.done()); I'm not really a friend of testing implementation details - we should be testing the public API, otherwise we will have to adjust the tests with each change of the internals. This should work for most of the tests here, these can be replaced by verifying that the selector `[foo] > :abp-has(div[foo="yxcv)'"]:not([bar='ddd'])) > [bar]` works correctly. Testing invalid selectors via the public API isn't possible unfortunately, error conditions won't be reported. Still, these should be tested via high-level APIs IMHO - testing ElemHideEmulation.patterns should do.
addressed both Sebastian and Wladimir comments. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:152: if (content == null) On 2017/06/08 08:28:47, Sebastian Noack wrote: > Nit: Given that parseSelectorContent() either returns and object or null, > perhaps `!content` is more appropriate here rather than `content == null`. Done. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing AdBlock Plus " + On 2017/06/08 08:28:48, Sebastian Noack wrote: > Nit: It's Adblock Plus (with lower case "B"). But perhaps we should avoid > mentioning the product name in error messages in the first place, in order to > avoid confusion in other products that are based on this code. (Same below.) Maybe we could change this to "Failed parsing content blocker selector". It is neutral. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:188: * @property {string} style CSS style represented a string. On 2017/06/08 13:12:40, Wladimir Palant wrote: > This doesn't seem to be quite English to me, also not very helpful. "sorted and > joined list of CSS properties" maybe? Note that this isn't a full sentence, so > there shouldn't be a period at the end. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:189: * @property {string[]} subSelectors selectors for the rule. On 2017/06/08 13:12:39, Wladimir Palant wrote: > "selectors that the CSS properties apply to"? Acknowledged. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:193: * Turn a CSSStyleRule into a StringifiedStyle On 2017/06/08 13:12:39, Wladimir Palant wrote: > You have the types listed below, no need to repeat them. "Produces a string > representation of a stylesheet entry." maybe? Note that this is a sentence, so > it should end with a period. Acknowledged. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:205: styles.push(property + ": " + value + (priority ? " !" + priority : "") + On 2017/06/08 08:28:47, Sebastian Noack wrote: > Nit: If you'd use a template literal here you woudnl't have to wrap this line: > > `${property}: ${value}${priority ? " !" + priority : ""};` This code was simply moved and written pre-ES6. I'll use a template string. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:35: document.body.removeChild(child); On 2017/06/08 13:12:42, Wladimir Palant wrote: > If you store the child in an intermediate variable, you should use it: > > child.parentNode.removeChild(child); Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:80: // insert a <div> with a unique id and and empty CSS rule On 2017/06/08 13:12:43, Wladimir Palant wrote: > You have "and" twice here. Also, how is this an empty CSS rule? It actually has > a value, according to the styleBlock parameter. Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:95: // and then will call the callback. On 2017/06/08 13:12:41, Wladimir Palant wrote: > There are no callbacks here, this function returns a promise. I forgot to update the comment when we switched this to promise. :-/ Done https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:97: // loop On 2017/06/08 13:12:42, Wladimir Palant wrote: > There is no loop here. But - yes, loadScript() might not detect the error > condition and reject the promise. Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:114: // instantiate a ElemHideEmulation with @selectors. On 2017/06/08 13:12:43, Wladimir Palant wrote: > "an ElemHideEmulation instance"? Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:140: return; On 2017/06/08 13:12:42, Wladimir Palant wrote: > This check is pointless, we can just let the loop run even with an empty array. Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:147: return Promise.resolve(); On 2017/06/08 13:12:43, Wladimir Palant wrote: > While it was like that before your changes already, it would probably make sense > to return Promise.resolve(elemHideEmulation) here. This way you can test > ElemHideEmulation properties where other options don't work. Done. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:169: }).catch(unexpectedError.bind(test)).then(() => test.done()); On 2017/06/08 13:12:40, Wladimir Palant wrote: > I'm not really a friend of testing implementation details - we should be testing > the public API, otherwise we will have to adjust the tests with each change of > the internals. This should work for most of the tests here, these can be > replaced by verifying that the selector `[foo] > > :abp-has(div[foo="yxcv)'"]:not([bar='ddd'])) > [bar]` works correctly. > > Testing invalid selectors via the public API isn't possible unfortunately, error > conditions won't be reported. Still, these should be tested via high-level APIs > IMHO - testing ElemHideEmulation.patterns should do. I actually think that *also* testing internal functions has a huge value. It allows easier detection of problems when changing things as it is more granular. It also serve as self documenting example of how it works. Yes they need to be fixed if we change the internals behaviour, but that's also true if we change public API. As you said above, there are even some test that are impossible with the public API, yet we want to ensure they work. I can remove line 159-164 as they were used to test quoting which is no longer done in this patch revision. I'll mark internals vs API so that we can distinguish.
https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elem... chrome/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing AdBlock Plus " + On 2017/06/08 15:45:48, hub wrote: > On 2017/06/08 08:28:48, Sebastian Noack wrote: > > Nit: It's Adblock Plus (with lower case "B"). But perhaps we should avoid > > mentioning the product name in error messages in the first place, in order to > > avoid confusion in other products that are based on this code. (Same below.) > > Maybe we could change this to "Failed parsing content blocker selector". It is > neutral. Well, we're actually parsing what we call an "ABP selector" (i.e. the :abp-* pseudo-classes) here, so I think we should call it that in the error message: Either "Adblock Plus selector" or "ABP selector". I personally prefer the latter since it seems easier to make the connection to the :abp-* pseudo-classes that way. Various products with different names use this code, but it's still an "ABP selector", similar to how we and others typically refer to the filter list syntax as something to the effect of "ABP-style filters".
Browser environment is no longer expected https://codereview.adblockplus.org/29383960/diff/29459612/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29459612/chrome/content/elem... chrome/content/elemHideEmulation.js:378: 0, "", document, cssStyles)) accessing document here will fail in Firefox. Will fix this.
This is good to land if you move reportError assignment into getFiltersFunc callback (quick and dirty solution indicated below). I still want to look into the rest of the unit tests after it lands however. https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/test/browser/elemHi... test/browser/elemHideEmulation.js:169: }).catch(unexpectedError.bind(test)).then(() => test.done()); On 2017/06/08 15:45:49, hub wrote: > I actually think that *also* testing internal functions has a huge value. It > allows easier detection of problems when changing things as it is more granular. As you wish but I certainly won't object to anybody removing these tests again when rewriting code. The point of unit tests is catching behavior changes that are visible outside, not flagging rewrites and requiring tests to be adapted despite unchanged behavior. In fact, that "anybody" might be me - once that code is moved to Emscripten, testing internals will no longer be feasible. https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elem... chrome/content/elemHideEmulation.js:331: reportError = error => this.window.console.error(error); This won't work. There might be multiple windows loading in parallel, you could end up overwriting this variable before it is used. I see who solution: 1) Quick and dirty: move this assignment into getFiltersFunc callback, all potential error reporting is happening synchronously there. 2) Proper: pass reportError function to the functions as parameter wherever necessary. I will accept 1) given the urgency but only if an issue is filed to implement 2).
https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elem... chrome/content/elemHideEmulation.js:331: reportError = error => this.window.console.error(error); On 2017/06/13 13:16:25, Wladimir Palant wrote: > This won't work. There might be multiple windows loading in parallel, you could > end up overwriting this variable before it is used. I see who solution: > > 1) Quick and dirty: move this assignment into getFiltersFunc callback, all > potential error reporting is happening synchronously there. > > 2) Proper: pass reportError function to the functions as parameter wherever > necessary. > > I will accept 1) given the urgency but only if an issue is filed to implement > 2). doing 1)
> > I will accept 1) given the urgency but only if an issue is filed to implement > > 2). > > doing 1) and filed https://issues.adblockplus.org/ticket/5313
felix gave me the go ahead to push this.
Message was sent while issue was closed.
On 2017/06/14 01:14:04, hub wrote: > felix gave me the go ahead to push this. Handling the tests in https://codereview.adblockplus.org/29464696/ 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
