Issue 4394 - Create a filter class for element hiding emulation filters [WIP]

Issue 4394 - Create a filter class for element hiding emulation filters

Repository: bitbucket.org/fhd/adblockpluscore

This is still WIP, I will close the review without landing this once we're done. I just want to make sure I'm going in the right direction here.

Once you confirm that, I will get this ready for a real review, including my changes for Chrome/Firefox support and some tests.

[Felix Dahlke, 2016-09-23 16:42:00]

Can you guys please give me some quick feedback on this? I just want to know
whether you think I'm moving in the right direction, would be a bummer to notice
this too late.

I've described my thinking and approach to this in the issue:
https://issues.adblockplus.org/ticket/4394

I've tested the code, but I won't put my Chrome/Firefox changes up for review
just yet.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:110: Filter.haspseudoclassRegExp = /:has\(/;
Didn't want to put too much energy into this regex at this stage, but I'll
revisit it.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:937: let hasSelectorUsed =
Filter.haspseudoclassRegExp.test(selector);
This code block is all about checking if the element hiding rule needs to be
emulated. Given the increasing complexity, I'd love to move this code into a
dedicated function (e.g. in ElemHideEmulationFilter). But for now I'm leaving it
here to make it easier to see what I've changed.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:938: let propertySelectorMatch =
Filter.propertyselectorRegExp.exec(selector);
I'm not really happy that we need to run every feature regex on every element
hiding filter this way. Ideas welcome.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:949: regexpSource = propertySelectorMatch[2];
These properties are only necessary for the CSSPropertyFilters content script.
Seems easiest to leave them here for now, since they will probably not be
necessary anymore once we tackle https://issues.adblockplus.org/ticket/3143.
OTOH, it's a bit messy to leave it this way until that issue is tackled. What do
you think?

https://codereview.adblockplus.org/29354827/diff/29354828/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/test/filterClasses....
test/filterClasses.js:331: exports.testElemHideEmulationFilters = function(test)
I'll add to these tests to make sure the parsing code works fine, but first I
wanted to finish this WIP review.

[kzar, 2016-09-25 14:52:07]

Sure, I've had a quick go. Mostly makes sense to me so far. (I'm a little
confused how the interaction between property filters and :has ones will work
however, which probably comes across in my filterClasses.js comments.)

Two general thoughts:

 - "Element hiding emulation filter" is kind of a mouth-full, but I don't have a
better suggestion!
 - I got in trouble for making both adblockpluscore and adblockplus{,chrome}
changes in the same issue in the past. Maybe add new issues for the related
adblockplus{,chrome} changes that are blocked by this one?

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
File chrome/content/cssProperties.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
chrome/content/cssProperties.js:131: // We currently don't support any element
hiding emulation feature except
Shouldn't it be up to the callback to decide which filters are used? (Since
that's in adblockpluschrome / adblockplus and support might depend on the
platform.)

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:938: let propertySelectorMatch =
Filter.propertyselectorRegExp.exec(selector);
On 2016/09/23 16:41:59, Felix Dahlke wrote:
> I'm not really happy that we need to run every feature regex on every element
> hiding filter this way. Ideas welcome.

Could we have a regexp which matches all element hiding emulation filters?

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:949: regexpSource = propertySelectorMatch[2];
On 2016/09/23 16:41:59, Felix Dahlke wrote:
> These properties are only necessary for the CSSPropertyFilters content script.
> Seems easiest to leave them here for now, since they will probably not be
> necessary anymore once we tackle https://issues.adblockplus.org/ticket/3143.
> OTOH, it's a bit messy to leave it this way until that issue is tackled. What
do
> you think?

But we still need the selector suffix and prefix for :has filters, so why would
we remove these? I guess I'm missing something.

[kzar, 2016-09-25 15:51:12]

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:944: return new InvalidFilter(text,
"filter_elemhideemulation_nodomain");
(I guess we'll have to update this string in adblockplus and then update the
adblockplus dependency in adblockpluschrome at the same time we make the other
changes in those repositories.)

[Felix Dahlke, 2016-09-30 10:22:27]

Thanks for getting back so fast Dave! And sorry for delaying it, I'd very much
like to move forward with this soon.

On 2016/09/25 14:52:07, kzar wrote:
>  - "Element hiding emulation filter" is kind of a mouth-full, but I don't have
a
> better suggestion!

Same here. I was originally going for "emulated element hiding filter" and then
"super element hiding filters" and "extended element hiding filters", but
ultimately Wladimir's suggestion ("element hiding emulation filter") seemed to
be most spot on.

No ideas/suggestions for something significantly shorter so far... What I could
imagine is to handle emulation within element hiding filters, without even using
a separate class. But that would make element hiding filters a pretty complex
affair...

>  - I got in trouble for making both adblockpluscore and adblockplus{,chrome}
> changes in the same issue in the past. Maybe add new issues for the related
> adblockplus{,chrome} changes that are blocked by this one?

Yeah sure, just didn't create the issues for that yet since I can't refer to the
revision of adblockpluscore to update to before my change has landed.

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
File chrome/content/cssProperties.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
chrome/content/cssProperties.js:131: // We currently don't support any element
hiding emulation feature except
On 2016/09/25 14:52:06, kzar wrote:
> Shouldn't it be up to the callback to decide which filters are used? (Since
> that's in adblockpluschrome / adblockplus and support might depend on the
> platform.)

Yeah, that's a good point. My thinking was that we can avoid duplication and
make things more robust by making sure the cssProperties content script ignores
all other filters, whether the platform specific code hands any in or not. Since
this content script will be replaced by one that can both handle :has and
property selectors, that seems to be the best option until we get there.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:938: let propertySelectorMatch =
Filter.propertyselectorRegExp.exec(selector);
On 2016/09/25 14:52:06, kzar wrote:
> On 2016/09/23 16:41:59, Felix Dahlke wrote:
> > I'm not really happy that we need to run every feature regex on every
element
> > hiding filter this way. Ideas welcome.
> 
> Could we have a regexp which matches all element hiding emulation filters?

Yeah, I think that should be possible. But it would be incredibly complex, as we
add more features, hence I much prefer separate regexps for readability's sake.
But I'm planning to run some performance tests to see how bad it really is, then
we can still change that.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:944: return new InvalidFilter(text,
"filter_elemhideemulation_nodomain");
On 2016/09/25 15:51:12, kzar wrote:
> (I guess we'll have to update this string in adblockplus and then update the
> adblockplus dependency in adblockpluschrome at the same time we make the other
> changes in those repositories.)

Yup, I was planning to roll out my adblockplus changes first, then
adblockpluschrome. I wonder whether we can just rename a translation string
without invalidating the existing translations on Crowdin - do you have
experience with that?

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:949: regexpSource = propertySelectorMatch[2];
On 2016/09/25 14:52:07, kzar wrote:
> On 2016/09/23 16:41:59, Felix Dahlke wrote:
> > These properties are only necessary for the CSSPropertyFilters content
script.
> > Seems easiest to leave them here for now, since they will probably not be
> > necessary anymore once we tackle https://issues.adblockplus.org/ticket/3143.
> > OTOH, it's a bit messy to leave it this way until that issue is tackled.
What
> do
> > you think?
> 
> But we still need the selector suffix and prefix for :has filters, so why
would
> we remove these? I guess I'm missing something.

Well, a filter can have both :has and property selectors - multiple times even,
in theory. So I believe this is going to change. But "believe" is the right
word, I'm not 100% sure how things are going to end up looking...

[Felix Dahlke, 2016-10-05 10:08:27]

Got Wladimir to do an in-person review, and he is fine with the overall
direction here, I added some of his comments which I'll address.

Dave, could you let me know if you're also happy with the general direction and
his comments (particularly about the feature detection part, this'll further
simplify things and make the intermediate step look a bit cleaner) now? Then I
can resume my work.

(As a side note, we realised that it's pretty messy to have this serialisation
logic in the message responder which isn't even in adblockpluscore, but in
adblockplusui. Wladimir is going to write an issue to improve the situation
there.)

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:110: Filter.haspseudoclassRegExp = /:has\(/;
On 2016/09/23 16:41:59, Felix Dahlke wrote:
> Didn't want to put too much energy into this regex at this stage, but I'll
> revisit it.

In-person comment from Wladimir: We probably don't need a regex match here, we
cannot use regex to parse the filter anyway. So we can just do a string match.

Idea from me as a result of this: We just do a string check for -abp-properties
as well, we move the parsing into the CPF content script for now.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:944: return new InvalidFilter(text,
"filter_elemhideemulation_nodomain");
On 2016/09/30 10:22:26, Felix Dahlke wrote:
> On 2016/09/25 15:51:12, kzar wrote:
> > (I guess we'll have to update this string in adblockplus and then update the
> > adblockplus dependency in adblockpluschrome at the same time we make the
other
> > changes in those repositories.)
> 
> Yup, I was planning to roll out my adblockplus changes first, then
> adblockpluschrome. I wonder whether we can just rename a translation string
> without invalidating the existing translations on Crowdin - do you have
> experience with that?

In-person comment from Wladimir: It's a different string anyway, there's no
reason to save the existing translations.

[kzar, 2016-10-05 11:58:15]

>Dave, could you let me know if you're also happy with the general direction and
his comments (particularly about the feature detection part, this'll further
simplify things and make the intermediate step look a bit cleaner) now? Then I
can resume my work.

Sure, there you go. Sorry for the delay. I think the general approach looks OK.
To be honest I usually get a proof of concept working for this kind of stuff
before I realise if my approach is good or bad, I find it hard to think of
everything in advance. So perhaps I missed something.

I do think the platform code needs to be able to decide which features it wants
to support. (Added an inline comment about that.) 

As for the name, well I can't think of anything much better either. Something
like "Advanced Element Hiding Filters" would be more catchy than "Element Hiding
Emulation Filters" but not as accurate. I guess "Content Script Filters" or
"Client Side Filters" could work? (Or "Borg Filters" as in "Resistance is
futile!" :p.)

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
File chrome/content/cssProperties.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
chrome/content/cssProperties.js:131: // We currently don't support any element
hiding emulation feature except
On 2016/09/30 10:22:26, Felix Dahlke wrote:
> On 2016/09/25 14:52:06, kzar wrote:
> > Shouldn't it be up to the callback to decide which filters are used? (Since
> > that's in adblockpluschrome / adblockplus and support might depend on the
> > platform.)
> 
> Yeah, that's a good point. My thinking was that we can avoid duplication and
> make things more robust by making sure the cssProperties content script
ignores
> all other filters, whether the platform specific code hands any in or not.
Since
> this content script will be replaced by one that can both handle :has and
> property selectors, that seems to be the best option until we get there.

How about adding an extra parameter to the CSSPropertyFilters constructor called
something like `features`? That way the platform code gets to pass in an array
of supported features and we can check the filters are supported here before
returning them.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:110: Filter.haspseudoclassRegExp = /:has\(/;
On 2016/10/05 10:08:26, Felix Dahlke wrote:
> On 2016/09/23 16:41:59, Felix Dahlke wrote:
> > Didn't want to put too much energy into this regex at this stage, but I'll
> > revisit it.
> 
> In-person comment from Wladimir: We probably don't need a regex match here, we
> cannot use regex to parse the filter anyway. So we can just do a string match.
> 
> Idea from me as a result of this: We just do a string check for
-abp-properties
> as well, we move the parsing into the CPF content script for now.

Acknowledged.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:937: let hasSelectorUsed =
Filter.haspseudoclassRegExp.test(selector);
On 2016/09/23 16:41:59, Felix Dahlke wrote:
> This code block is all about checking if the element hiding rule needs to be
> emulated. Given the increasing complexity, I'd love to move this code into a
> dedicated function (e.g. in ElemHideEmulationFilter). But for now I'm leaving
it
> here to make it easier to see what I've changed.

Yea I agree, if you don't move this into a separate function at least put a
comment for the block of code explaining the general idea.

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:949: regexpSource = propertySelectorMatch[2];
On 2016/09/30 10:22:26, Felix Dahlke wrote:
> On 2016/09/25 14:52:07, kzar wrote:
> > On 2016/09/23 16:41:59, Felix Dahlke wrote:
> > > These properties are only necessary for the CSSPropertyFilters content
> script.
> > > Seems easiest to leave them here for now, since they will probably not be
> > > necessary anymore once we tackle
https://issues.adblockplus.org/ticket/3143.
> > > OTOH, it's a bit messy to leave it this way until that issue is tackled.
> What
> > do
> > > you think?
> > 
> > But we still need the selector suffix and prefix for :has filters, so why
> would
> > we remove these? I guess I'm missing something.
> 
> Well, a filter can have both :has and property selectors - multiple times
even,
> in theory. So I believe this is going to change. But "believe" is the right
> word, I'm not 100% sure how things are going to end up looking...

OK fair enough, me neither to be honest.

[Felix Dahlke, 2016-11-03 16:06:54]

Superseded by https://codereview.adblockplus.org/29361668/

[Felix Dahlke, 2016-11-03 16:14:13]

Whoops, forgot to respond to your last batch of comments. Here you go :) As for
the name, that kinda stuck, I don't think we can do much better...

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
File chrome/content/cssProperties.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/chrome/content/cssP...
chrome/content/cssProperties.js:131: // We currently don't support any element
hiding emulation feature except
On 2016/10/05 11:58:14, kzar wrote:
> On 2016/09/30 10:22:26, Felix Dahlke wrote:
> > On 2016/09/25 14:52:06, kzar wrote:
> > > Shouldn't it be up to the callback to decide which filters are used?
(Since
> > > that's in adblockpluschrome / adblockplus and support might depend on the
> > > platform.)
> > 
> > Yeah, that's a good point. My thinking was that we can avoid duplication and
> > make things more robust by making sure the cssProperties content script
> ignores
> > all other filters, whether the platform specific code hands any in or not.
> Since
> > this content script will be replaced by one that can both handle :has and
> > property selectors, that seems to be the best option until we get there.
> 
> How about adding an extra parameter to the CSSPropertyFilters constructor
called
> something like `features`? That way the platform code gets to pass in an array
> of supported features and we can check the filters are supported here before
> returning them.

My thinking on this was that we don't have to pass anything in, we can just do
feature detection in ElemHideBase.fromText. But for now this is a theoretical
discussion anyway, since it'll take quite a while before :has will be supported
natively...

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.j...
lib/filterClasses.js:937: let hasSelectorUsed =
Filter.haspseudoclassRegExp.test(selector);
On 2016/10/05 11:58:14, kzar wrote:
> On 2016/09/23 16:41:59, Felix Dahlke wrote:
> > This code block is all about checking if the element hiding rule needs to be
> > emulated. Given the increasing complexity, I'd love to move this code into a
> > dedicated function (e.g. in ElemHideEmulationFilter). But for now I'm
leaving
> it
> > here to make it easier to see what I've changed.
> 
> Yea I agree, if you don't move this into a separate function at least put a
> comment for the block of code explaining the general idea.

The code is quite a bit shorter now that I reduced this to detection logic and
moved all the parsing stuff to the content script. So I figured it's fine
without comment/function for now. Have a look at the new code and let me know
what you think.