Issue 29361668: Issue 4394 - Create a filter class for element hiding emulation filters (Closed)
|
Created:
Nov. 3, 2016, 3:42 p.m. by Felix Dahlke Modified:
Nov. 22, 2016, 11 a.m. Base URL:
https://bitbucket.org/fhd/adblockpluscore Visibility:
Public. |
DescriptionIssue 4394 - Create a filter class for element hiding emulation filters
Patch Set 1 #
Total comments: 36
Patch Set 2 : Address kzar's comments #Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Address Wladimir's comments #
Total comments: 8
Patch Set 5 : Address kzar's comments #Patch Set 6 : Improve compliance with the 80 column rule #
MessagesTotal messages: 28
Some pointers: 1. The biggest part here is a renaming from CSS Property Filter to Element Hiding Emulation Filter. 2. I moved the property selector parsing logic to the content script because in the next step we will have to do this at runtime anyway, and it's cleaner this way. 3. I introduced some code that is being shared between background page and content scripts: shared.js. So far we didn't have any of that, but I wanted to avoid duplication. 4. I added some basic tests for the feature detection logic, but at the same time the content script is largely untested. I think we should tackle this when working on https://issues.adblockplus.org/ticket/3143. What I did to test this: 1. I integrated this in both Chrome and Firefox and tested it on testpages.adblockplus.org. I will upload those patches once this one landed. 2. I did some performance testing in Chrome to ensure that filter loading isn't slowed down by this - it isn't. 3. I verified that the JsDoc references to the globals in shared.js work.
(Just looking through these reviews today, sorry for the delay.) On 2016/11/03 15:58:50, Felix Dahlke wrote: > content scripts: shared.js. So far we didn't have any of that, but I wanted to > avoid duplication. In adblockpluschrome we call code that's shared between the background and content scripts `common.js`, perhaps we do the same here for consistency?
On 2016/11/04 13:20:00, kzar wrote: > (Just looking through these reviews today, sorry for the delay.) > > On 2016/11/03 15:58:50, Felix Dahlke wrote: > > content scripts: shared.js. So far we didn't have any of that, but I wanted to > > avoid duplication. > In adblockpluschrome we call code that's shared between the background and > content scripts `common.js`, perhaps we do the same here for consistency? Sure, but I'll wait with changes until you're done with the first review round - I've got a feeling there will be more comments :)
https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... chrome/content/elemHideEmulation.js:134: if (pattern.features != elemHideEmulationFeatureMap.PROPERTY_SELECTOR) Before I suggested adding a supportedFeatures parameters to the ElemHideEmulation constructor but I had another idea. How about we turn the parameters into a single options Object, so things like getFiltersFunc and addSelectorsFunc are properties on that? Since most features will require some special functions to be passed in I suppose we can make them all optional and then only enable features for which the functions have been provided. ElemHideEmulation instances could then have a supportedFeatures property which is assigned based on the functions provided to the constructor. Then we could do something like `if (pattern.features & ~this.supportedFeatures)` to check if any unsupported features are required for the filter. https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... chrome/content/elemHideEmulation.js:138: var match = propertySelectorRegExp.exec(pattern.selector); I guess the regexp won't necessarily match and if it doesn't we should continue too? https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) The feature flags are a nice idea but I wonder what will happen if we add a new feature in the future. Since the old versions of ElemHideBase.fromText won't have any logic to detect a new and unsupported feature it won't assign the feature flag for it either. So then the content script won't know that a new and unsupported feature has been used for a filter and it could barf when attempting to parse and apply it. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:928: return new ElemHideEmulationFilter(text, domain, selector, I wonder if it's really a good idea to move the logic for parsing element hiding emulation filters to the content script? Especially since when :has support is added the parsing logic will be even more complex. Perhaps the parsing logic should stay here like it does for all other types of filters? I guess I'm curious what your reasoning for moving it to the content script is? (You mention it's cleaner that way and we'll need to do this anyway, but that's kind of vague.) Maybe it'd help to share your WIP changes to adblockpluschrome so I can get my head around it all a bit better? https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:992: * Features used in this filter, combination of values from Perhaps describe it as bit flags instead of combination of values? https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js File lib/shared.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js#newco... lib/shared.js:35: .replace(/\^\|$/, "^") // remove anchors following separator placeholder Nit: Mind fixing these long lines? https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... test/elemHideEmulation.js:31: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), Nit: Mind fixing the few long lines here? https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") Nit: Also mind fixing some of these long lines? (Not the compareFilter ones.) https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.... test/filterClasses.js:366: compareFilter(test, "foo.com##:has([-abp-properties='abc'])", ["type=elemhideemulation", "text=foo.com##:has([-abp-properties='abc'])", "selectorDomain=foo.com", "selector=:has([-abp-properties='abc'])", "domains=FOO.COM", "features=3"]); Please don't hardcode the feature flag numbers, better to use the feature map here too.
I've responded to all comments, but I haven't uploaded a new patch set yet, since there may be more changes and I'd rather tackle them in one go. There's also the shared.js->common.js renaming which I'll do then. https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... chrome/content/elemHideEmulation.js:134: if (pattern.features != elemHideEmulationFeatureMap.PROPERTY_SELECTOR) On 2016/11/04 15:45:56, kzar wrote: > Before I suggested adding a supportedFeatures parameters to the > ElemHideEmulation constructor but I had another idea. > > How about we turn the parameters into a single options Object, so things like > getFiltersFunc and addSelectorsFunc are properties on that? Since most features > will require some special functions to be passed in I suppose we can make them > all optional and then only enable features for which the functions have been > provided. ElemHideEmulation instances could then have a supportedFeatures > property which is assigned based on the functions provided to the constructor. > > Then we could do something like `if (pattern.features & > ~this.supportedFeatures)` to check if any unsupported features are required for > the filter. I'm not sure. For one, I feel that logic belongs in filterClasses: A filter is an ElemHideEmulation filter if it has features that need to be emulated. A filter is an ElemHide filter if its syntax is natively supported by the browser. In that case we wouldn't even have to run this content script. Secondly, while I tried to make sure we already have an idea for how we could deal with features that are natively supported in some browsers but not all, for now this is a completely theoretical discussion since -abp-properties is a bit unlikely to be natively supported anywhere, and :has is not supported in any browser, it's still just a draft. IMO this is something we can deal with much later. https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... chrome/content/elemHideEmulation.js:138: var match = propertySelectorRegExp.exec(pattern.selector); On 2016/11/04 15:45:56, kzar wrote: > I guess the regexp won't necessarily match and if it doesn't we should continue > too? Oops, true, I'll fix that. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/04 15:45:56, kzar wrote: > The feature flags are a nice idea but I wonder what will happen if we add a new > feature in the future. Since the old versions of ElemHideBase.fromText won't > have any logic to detect a new and unsupported feature it won't assign the > feature flag for it either. So then the content script won't know that a new and > unsupported feature has been used for a filter and it could barf when attempting > to parse and apply it. Well, if we detect NO feature here, this would just become a regular element hiding filter, so the browser will have to deal with the unsupported syntax. I guess the situation you mean is that there could be a filter that has one supported feature, but then something that is really just an invalid CSS selector, which could confuse our parsing code in the elemHideEmulation content script? I guess the best we can do is make sure that parsing logic is robust against invalid syntax? https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:928: return new ElemHideEmulationFilter(text, domain, selector, On 2016/11/04 15:45:56, kzar wrote: > I wonder if it's really a good idea to move the logic for parsing element hiding > emulation filters to the content script? Especially since when :has support is > added the parsing logic will be even more complex. Perhaps the parsing logic > should stay here like it does for all other types of filters? > > I guess I'm curious what your reasoning for moving it to the content script is? > (You mention it's cleaner that way and we'll need to do this anyway, but that's > kind of vague.) Maybe it'd help to share your WIP changes to adblockpluschrome > so I can get my head around it all a bit better? Well, one part of the truth is that I don't know how the parsing logic is going to work yet. Are we going to have a separate parsing step, or are we going to interpret the filter on the fly? I think we should leave that decision to the content script, I don't feel it belongs here. In addition, having complex pre-parsing logic here can slow down filter loading. I'd rather do this lazily and slow down the content script, which is only active on sites where we need those filters anyway. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:992: * Features used in this filter, combination of values from On 2016/11/04 15:45:56, kzar wrote: > Perhaps describe it as bit flags instead of combination of values? I thought I'd keep this in line with the doc comment for the only other bitmap we have here, which is RegExpFilter.contentType. https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js File lib/shared.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js#newco... lib/shared.js:35: .replace(/\^\|$/, "^") // remove anchors following separator placeholder On 2016/11/04 15:45:56, kzar wrote: > Nit: Mind fixing these long lines? Well, I tried to leave the code alone since I'm just moving it, but yeah, why not fix this while at it. I'll do that in the next patch set. https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... test/elemHideEmulation.js:31: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), On 2016/11/04 15:45:56, kzar wrote: > Nit: Mind fixing the few long lines here? It bothered me too, but I don't see an obvious way of doing that here. From the look of it we generally seem to ignore the 80 column rule in require lines with destructuring assignments. How would you do it? https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") On 2016/11/04 15:45:56, kzar wrote: > Nit: Also mind fixing some of these long lines? (Not the compareFilter ones.) See my comment in elemHideEmulation.js. https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.... test/filterClasses.js:366: compareFilter(test, "foo.com##:has([-abp-properties='abc'])", ["type=elemhideemulation", "text=foo.com##:has([-abp-properties='abc'])", "selectorDomain=foo.com", "selector=:has([-abp-properties='abc'])", "domains=FOO.COM", "features=3"]); On 2016/11/04 15:45:56, kzar wrote: > Please don't hardcode the feature flag numbers, better to use the feature map > here too. Good point, will do.
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) I guess my question is what's the point of having the features flags at all? On the surface having it sounds like a good idea, but I'm not sure how it helps anything. If we add a new emulation feature in the future the old code won't know how to detect the feature anyway. It only does anything here as we're getting ahead of ourselves and adding the :has detection before adding :has support. (Which arguably is an unrelated change.) Wouldn't it make more sense to just consider a filter an emulation filter or not? Especially since we apparently don't intend to leave it up to the platform which emulation filter features are used.
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 12:36:26, kzar wrote: > I guess my question is what's the point of having the features flags at all? On > the surface having it sounds like a good idea, but I'm not sure how it helps > anything. If we add a new emulation feature in the future the old code won't > know how to detect the feature anyway. It only does anything here as we're > getting ahead of ourselves and adding the :has detection before adding :has > support. (Which arguably is an unrelated change.) > > Wouldn't it make more sense to just consider a filter an emulation filter or > not? Especially since we apparently don't intend to leave it up to the platform > which emulation filter features are used. Well, we have to check for emulated features before we create the filter object - otherwise we wouldn't know whether it's an emulation filter or not. We could of course just set a single boolean instead of setting feature flags, then we could determine what features are being used in the content script. Right now the main benefit of having the feature flags here is that it makes testing and debugging easier. Note that we're not getting ahead of ourselves here by detecting :has - #4394 is a direct prerequisite of #3143, it doesn't make any sense in isolation actually. It was just too big for a single issue in my mind, that's all. This way, #3143 is only going to be a change to the content script, won't touch anything else. That makes sense, since it's going to be a somewhat complex change.
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 14:41:24, Felix Dahlke wrote: > We could of course just set a single boolean instead of setting feature flags Well we don't need to set a boolean flag here either, we would simply return a new ElemHideEmulationFilter instead of a new ElemHideFilter. > then we could determine what features are being used in the content script. Isn't that more or less what you're planning to do anyway, parsing and handling emulation filters in the content script? If we have a filter with nested features we'll have to do more work to find and split them out then (possibly recursively), so I guess I don't see the utility of figuring out the features an emulation filter uses here too. (The only advantage I can see would be that filters using unsupported features could be ignored, except that they can't since the logic here doesn't know how to set the feature flags for those anyway.)
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 15:59:44, kzar wrote: > On 2016/11/07 14:41:24, Felix Dahlke wrote: > > We could of course just set a single boolean instead of setting feature flags > > Well we don't need to set a boolean flag here either, we would simply return a > new ElemHideEmulationFilter instead of a new ElemHideFilter. Yeah of course, my bad. > > then we could determine what features are being used in the content script. > > Isn't that more or less what you're planning to do anyway, parsing and handling > emulation filters in the content script? If we have a filter with nested > features we'll have to do more work to find and split them out then (possibly > recursively), so I guess I don't see the utility of figuring out the features an > emulation filter uses here too. > > (The only advantage I can see would be that filters using unsupported features > could be ignored, except that they can't since the logic here doesn't know how > to set the feature flags for those anyway.) Feature detection are simple string checks. Note that we have to do the checking here no matter what, the question is whether we record the results or not. To make testing and debugging easier I'd prefer to keep the flags. But I guess we'll have to agree to disagree on this, too, let's just wait for Wladimir.
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 16:51:24, Felix Dahlke wrote: > Feature detection are simple string checks. Note that we have to do the > checking here no matter what, the question is whether we record the > results or not. That's not quite true, since if we're recording used features here we need to test for all known patterns. If we just want to decide if a filter is a emulation filter then we "only" need to search for the presence of any feature. (By the way I wasn't trying to say that I am definitely right and that having the feature flags for emulation filters definitely isn't useful. Just that I realised it doesn't actually help us much to check for feature support for only the features we know and therefore probably support anyway! But if you can't see what it's for either perhaps we should just remove it, no point having extra complexity that doesn't have a purpose.)
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:928: return new ElemHideEmulationFilter(text, domain, selector, On 2016/11/04 16:43:35, Felix Dahlke wrote: >I'd rather do this lazily and slow down the content script, which is only > active on sites where we need those filters anyway. That's a pretty good point actually, I didn't think of that. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:992: * Features used in this filter, combination of values from On 2016/11/04 16:43:35, Felix Dahlke wrote: > I thought I'd keep this in line with the doc comment for the only other > bitmap we have here, which is RegExpFilter.contentType. Well bitmap works too. (I guess if you're worried about consistency you could improve the other comment too.) https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... test/elemHideEmulation.js:31: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), On 2016/11/04 16:43:35, Felix Dahlke wrote: > How would you do it? Well this line is not the worst, but none the less I would probably do it like so: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), I suppose I would adjust the later line like so: let matches = ElemHideEmulation.getRulesForDomain(domain).map( filter => filter.text );
Some comment responses, but I guess I have enough now to work on the next patch set, will do soon. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/07 17:10:22, kzar wrote: > On 2016/11/07 16:51:24, Felix Dahlke wrote: > > Feature detection are simple string checks. Note that we have to do the > > checking here no matter what, the question is whether we record the > > results or not. > > That's not quite true, since if we're recording used features here we need to > test for all known patterns. If we just want to decide if a filter is a > emulation filter then we "only" need to search for the presence of any feature. Well, sure, we don't have to run all checks if one matches unless we go for flags. I doubt this has a relevant impact on performance however - this is only problematic if we have a large number of emulation filters. The complexity / amount of code is about the same. > > (By the way I wasn't trying to say that I am definitely right and that having > the feature flags for emulation filters definitely isn't useful. Just that I > realised it doesn't actually help us much to check for feature support for only > the features we know and therefore probably support anyway! But if you can't see > what it's for either perhaps we should just remove it, no point having extra > complexity that doesn't have a purpose.) I can absolutely see your point. The flags are currently only needed to test the detection logic. Having code that is only useful for tests is generally a bit of a red flag to me (although it does sometimes make sense). I'm planning to add proper tests for the content script with #3143. So then we could test everything there and, if it turns out we don't need the flags for anything else, get rid of them for good. However, for #4394 I would kind of prefer to leave them in for the sake of having these tests. For this I would add a comment to the flags that explains this situation, and also mention it in #3143 as something we need to look into. Does that make sense? https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... test/elemHideEmulation.js:31: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), On 2016/11/07 17:19:26, kzar wrote: > On 2016/11/04 16:43:35, Felix Dahlke wrote: > > How would you do it? > > Well this line is not the worst, but none the less I would probably do it like > so: > > {Filter, > ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), > > I suppose I would adjust the later line like so: > > let matches = ElemHideEmulation.getRulesForDomain(domain).map( > filter => filter.text > ); Yeah, that's nice actually.
New patch set is up, all comments addressed AFAICT. Note that I haven't rebased this on top of #4593, since I want to do this in one go once #4600 has landed. So no regexp or case insensitive matching yet. https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/chrome/content/elem... chrome/content/elemHideEmulation.js:138: var match = propertySelectorRegExp.exec(pattern.selector); On 2016/11/04 16:43:35, Felix Dahlke wrote: > On 2016/11/04 15:45:56, kzar wrote: > > I guess the regexp won't necessarily match and if it doesn't we should > continue > > too? > > Oops, true, I'll fix that. Done. https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js File lib/shared.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/shared.js#newco... lib/shared.js:35: .replace(/\^\|$/, "^") // remove anchors following separator placeholder On 2016/11/04 16:43:35, Felix Dahlke wrote: > On 2016/11/04 15:45:56, kzar wrote: > > Nit: Mind fixing these long lines? > > Well, I tried to leave the code alone since I'm just moving it, but yeah, why > not fix this while at it. I'll do that in the next patch set. Done. https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/elemHideEmulat... test/elemHideEmulation.js:31: {Filter, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), On 2016/11/08 17:45:35, Felix Dahlke wrote: > On 2016/11/07 17:19:26, kzar wrote: > > On 2016/11/04 16:43:35, Felix Dahlke wrote: > > > How would you do it? > > > > Well this line is not the worst, but none the less I would probably do it like > > so: > > > > {Filter, > > ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses"), > > > > I suppose I would adjust the later line like so: > > > > let matches = ElemHideEmulation.getRulesForDomain(domain).map( > > filter => filter.text > > ); > > Yeah, that's nice actually. Done. https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/test/filterClasses.... test/filterClasses.js:366: compareFilter(test, "foo.com##:has([-abp-properties='abc'])", ["type=elemhideemulation", "text=foo.com##:has([-abp-properties='abc'])", "selectorDomain=foo.com", "selector=:has([-abp-properties='abc'])", "domains=FOO.COM", "features=3"]); On 2016/11/04 16:43:35, Felix Dahlke wrote: > On 2016/11/04 15:45:56, kzar wrote: > > Please don't hardcode the feature flag numbers, better to use the feature map > > here too. > > Good point, will do. Done.
> Note that I haven't rebased this on top of #4593, since I > want to do this in one go once #4600 has landed. Mind rebasing now that it landed (as a separate patch set)? https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/08 17:45:35, Felix Dahlke wrote: > I'm planning to add proper tests for the content script with #3143. So > then we could test everything there and, if it turns out we don't need > the flags for anything else, get rid of them for good. However, for #4394 > I would kind of prefer to leave them in for the sake of having these > tests. Well no I think that if the only reason to add all this complexity is for the unit tests then we shouldn't have it. If you want to check if a filter contains ":has" or "[-abp-properties" you can easily do that in the tests. (If it's less convenient we could have a utility function that does that in the tests too.) (I've always been told to make things as simple as possible in code reviews, only adding things when needed. This seems like the opposite logic, let's keep it in since it looks nice superficially and maybe we'll need it one day.) > Note that we're not getting ahead of ourselves here by detecting :has - > #4394 is a direct prerequisite of #3143, it doesn't make any sense in > isolation actually. It was just too big for a single issue in my mind, > that's all. This way, #3143 is only going to be a change to the content > script, won't touch anything else. That makes sense, since it's going > to be a somewhat complex change. I still think we should remove the `if (selector.indexOf(":has(") != -1)` check since we don't support those filters yet. I think that check should be added at the same time :has selector support is added. So what if two extra lines in here get added at the same time as a content script is modified? If anything that would be clearer since the change would be titled something like "Add :has support to elemhide emulation filters" and all the changes for that are grouped together.
Rebased, I believe everything has been addressed now, except for the open discussion where we need Wladimir's opinion. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/15 16:54:13, kzar wrote: > On 2016/11/08 17:45:35, Felix Dahlke wrote: > > I'm planning to add proper tests for the content script with #3143. So > > then we could test everything there and, if it turns out we don't need > > the flags for anything else, get rid of them for good. However, for #4394 > > I would kind of prefer to leave them in for the sake of having these > > tests. > > Well no I think that if the only reason to add all this complexity is for the > unit tests then we shouldn't have it. If you want to check if a filter contains > ":has" or "[-abp-properties" you can easily do that in the tests. (If it's less > convenient we could have a utility function that does that in the tests too.) > > (I've always been told to make things as simple as possible in code reviews, > only adding things when needed. This seems like the opposite logic, let's keep > it in since it looks nice superficially and maybe we'll need it one day.) > > Note that we're not getting ahead of ourselves here by detecting :has - > > #4394 is a direct prerequisite of #3143, it doesn't make any sense in > > isolation actually. It was just too big for a single issue in my mind, > > that's all. This way, #3143 is only going to be a change to the content > > script, won't touch anything else. That makes sense, since it's going > > to be a somewhat complex change. > > I still think we should remove the `if (selector.indexOf(":has(") != -1)` check > since we don't support those filters yet. I think that check should be added at > the same time :has selector support is added. So what if two extra lines in here > get added at the same time as a content script is modified? If anything that > would be clearer since the change would be titled something like "Add :has > support to elemhide emulation filters" and all the changes for that are grouped > together. I still think it makes sense to start detecting (yet ignoring) filters that use :has now, as part of #4394 to avoid undefined behaviour and restrict #3143 to rewriting the content script. Bear in mind that this is a preparatory change for #3143 that is not meant to make any sense in isolation. We can either duplicate the check in filterClasses and the content script or go for the flag, I prefer the latter. But we're moving in circles again, so let's leave this one for Wladimir.
On 2016/11/15 21:11:45, Felix Dahlke wrote: > But we're moving in circles again, so let's leave this one for Wladimir. Fair enough, but I worry I'm annoying you and not contributing much to these reviews. If most things get deferred to Wladimir perhaps we should cut out the middle man (me) and go straight to him next time?
On 2016/11/16 10:53:40, kzar wrote: > On 2016/11/15 21:11:45, Felix Dahlke wrote: > > But we're moving in circles again, so let's leave this one for Wladimir. > > Fair enough, but I worry I'm annoying you and not contributing much to these > reviews. If most things get deferred to Wladimir perhaps we should cut out the > middle man (me) and go straight to him next time? Well there's just one point we disagree about, but in general I find your reviews valuable and I'm sure it makes things easier for Wladimir too. I'm honestly curious how he feels about our disagreement here because I'm personally a bit undecided.
On 2016/11/16 10:59:18, Felix Dahlke wrote: > but in general I find your reviews valuable and I'm sure it makes things easier > for Wladimir too. Phew OK, good to know :)
https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) I actually have to agree with Dave here. You are doing preparation work for the :has selector, sure. But you are not adding support for it yet - so it makes no sense to detect it at this point. Right now :has should still be treated as a normal CSS selector meaning that it will be ignored by the browser's CSS engine. Also, I agree that the features bitmask is unnecessary. The content script will apply its regexp anyway, so it will detect this way whether the filter is a proper CSS property filter. And unit tests can still test recognition of emulation filters indirectly - the filterListener unit test will see these filters being sorted into a different container. https://codereview.adblockplus.org/29361668/diff/29362593/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29362593/chrome/content/elem... chrome/content/elemHideEmulation.js:137: var propertySelectorRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; This regexp should be created outside the loop, ideally even outside the function. https://codereview.adblockplus.org/29361668/diff/29362593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29362593/lib/filterClasses.j... lib/filterClasses.js:921: if (emulatedFeatures != 0) Nit: `if (emulatedFeatures)`?
New patch set is up, all comments should be addressed. https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29361669/lib/filterClasses.j... lib/filterClasses.js:918: if (selector.indexOf(":has(") != -1) On 2016/11/21 11:39:14, Wladimir Palant wrote: > I actually have to agree with Dave here. You are doing preparation work for the > :has selector, sure. But you are not adding support for it yet - so it makes no > sense to detect it at this point. Right now :has should still be treated as a > normal CSS selector meaning that it will be ignored by the browser's CSS engine. > > Also, I agree that the features bitmask is unnecessary. The content script will > apply its regexp anyway, so it will detect this way whether the filter is a > proper CSS property filter. And unit tests can still test recognition of > emulation filters indirectly - the filterListener unit test will see these > filters being sorted into a different container. Fair enough, I guess if we're not worried about backwards compatibility there's really no reason to deal with :has at all, at this stage. Simplifies everything quite a bit for now. https://codereview.adblockplus.org/29361668/diff/29362593/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29361668/diff/29362593/chrome/content/elem... chrome/content/elemHideEmulation.js:137: var propertySelectorRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/; On 2016/11/21 11:39:15, Wladimir Palant wrote: > This regexp should be created outside the loop, ideally even outside the > function. Done. https://codereview.adblockplus.org/29361668/diff/29362593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29362593/lib/filterClasses.j... lib/filterClasses.js:921: if (emulatedFeatures != 0) On 2016/11/21 11:39:15, Wladimir Palant wrote: > Nit: `if (emulatedFeatures)`? Removed along with the feature bitmask.
https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js#newco... lib/common.js:49: if (typeof exports != "undefined") Nit: The braces here aren't required. https://codereview.adblockplus.org/29361668/diff/29363652/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/filterClasses.j... lib/filterClasses.js:916: selector.indexOf(":has(") != -1) You're still checking for ":has". https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") Nit: This and some of the other lines you've modified in this file have become / are too long. Mind fixing them? For example: ( {Filter, InvalidFilter, CommentFilter, ActiveFilter, RegExpFilter, BlockingFilter, WhitelistFilter, ElemHideBase, ElemHideFilter, ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") ... if (type == "whitelist" || type == "filterlist" || type == "elemhide" || type == "elemhideexception" || type == "elemhideemulation") ... if (type == "elemhide" || type == "elemhideexception" || type == "elemhideemulation") ... test.equal(typeof ElemHideEmulationFilter, "function", "typeof ElemHideEmulationFilter"); The compareFilter lines probably aren't worth fixing, but you could try something like this for those: compareFilter( test, "foo.com##aaa [-abp-properties='abc'] bbb", ["type=elemhideemulation", "text=foo.com##aaa [-abp-properties='abc'] bbb", "selectorDomain=foo.com", "selector=aaa [-abp-properties='abc'] bbb", "domains=FOO.COM"] );
Thanks Dave, new patch set is up! https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/common.js#newco... lib/common.js:49: if (typeof exports != "undefined") On 2016/11/21 16:24:55, kzar wrote: > Nit: The braces here aren't required. Done. https://codereview.adblockplus.org/29361668/diff/29363652/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/lib/filterClasses.j... lib/filterClasses.js:916: selector.indexOf(":has(") != -1) On 2016/11/21 16:24:55, kzar wrote: > You're still checking for ":has". Done. https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") On 2016/11/21 16:24:55, kzar wrote: > Nit: This and some of the other lines you've modified in this file have become / > are too long. Mind fixing them? For example: > > ( > {Filter, InvalidFilter, CommentFilter, ActiveFilter, RegExpFilter, > BlockingFilter, WhitelistFilter, > ElemHideBase, ElemHideFilter, ElemHideException, > ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") > > ... > > if (type == "whitelist" || type == "filterlist" || type == "elemhide" || > type == "elemhideexception" || type == "elemhideemulation") > > ... > > if (type == "elemhide" || type == "elemhideexception" || > type == "elemhideemulation") > > ... > > test.equal(typeof ElemHideEmulationFilter, "function", > "typeof ElemHideEmulationFilter"); > > The compareFilter lines probably aren't worth fixing, but you could try > something like this for those: > > compareFilter( > test, > "foo.com##aaa [-abp-properties='abc'] bbb", > ["type=elemhideemulation", > "text=foo.com##aaa [-abp-properties='abc'] bbb", > "selectorDomain=foo.com", > "selector=aaa [-abp-properties='abc'] bbb", > "domains=FOO.COM"] > ); I'd actually prefer to leave those alone as part of this patch set, I don't want the actual changes to go under. Could be done in a separate noissue.
https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") On 2016/11/21 17:26:12, Felix Dahlke wrote: > I'd actually prefer to leave those alone as part of this patch set, I > don't want the actual changes to go under. Could be done in a separate > noissue. Fair enough, but IMO you should at least fix the lines which are now too long but weren't before your changes.
New patch set is up. https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29361668/diff/29363652/test/filterClasses.... test/filterClasses.js:43: ElemHideException, ElemHideEmulationFilter} = sandboxedRequire("../lib/filterClasses") On 2016/11/21 19:18:22, kzar wrote: > On 2016/11/21 17:26:12, Felix Dahlke wrote: > > I'd actually prefer to leave those alone as part of this patch set, I > > don't want the actual changes to go under. Could be done in a separate > > noissue. > > Fair enough, but IMO you should at least fix the lines which are now too long > but weren't before your changes. Fair enough, I fixed all lines I'm touching know, except for the compareFilter() ones at the bottom. I actually found those a bit easier to deal with without wrapping, so this seems like something we should touch in a separate commit.
LGTM
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
