Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29354827: Issue 4394 - Create a filter class for element hiding emulation filters [WIP] (Closed)

Sept. 23, 2016, 4:21 p.m. by Felix Dahlke
Nov. 3, 2016, 4:14 p.m.
kzar, Wladimir Palant


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.

Patch Set 1 #

Total comments: 21
Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -51 lines) Patch
M chrome/content/cssProperties.js View 1 chunk +12 lines, -1 line 4 comments Download
M lib/elemHideEmulation.js View 1 chunk +8 lines, -8 lines 0 comments Download
M lib/filterClasses.js View 5 chunks +59 lines, -20 lines 16 comments Download
M lib/filterListener.js View 4 chunks +7 lines, -7 lines 0 comments Download
M test/filterClasses.js View 7 chunks +15 lines, -15 lines 1 comment Download


Total messages: 9
Felix Dahlke
Sept. 23, 2016, 4:21 p.m. (2016-09-23 16:21:13 UTC) #1
Felix Dahlke
Can you guys please give me some quick feedback on this? I just want to ...
Sept. 23, 2016, 4:42 p.m. (2016-09-23 16:42:00 UTC) #2
Sure, I've had a quick go. Mostly makes sense to me so far. (I'm a ...
Sept. 25, 2016, 2:52 p.m. (2016-09-25 14:52:07 UTC) #3
https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29354827/diff/29354828/lib/filterClasses.js#newcode944 lib/filterClasses.js:944: return new InvalidFilter(text, "filter_elemhideemulation_nodomain"); (I guess we'll have to ...
Sept. 25, 2016, 3:51 p.m. (2016-09-25 15:51:12 UTC) #4
Felix Dahlke
Thanks for getting back so fast Dave! And sorry for delaying it, I'd very much ...
Sept. 30, 2016, 10:22 a.m. (2016-09-30 10:22:27 UTC) #5
Felix Dahlke
Got Wladimir to do an in-person review, and he is fine with the overall direction ...
Oct. 5, 2016, 10:08 a.m. (2016-10-05 10:08:27 UTC) #6
>Dave, could you let me know if you're also happy with the general direction and ...
Oct. 5, 2016, 11:58 a.m. (2016-10-05 11:58:15 UTC) #7
Felix Dahlke
Superseded by https://codereview.adblockplus.org/29361668/
Nov. 3, 2016, 4:06 p.m. (2016-11-03 16:06:54 UTC) #8
Felix Dahlke
Nov. 3, 2016, 4:14 p.m. (2016-11-03 16:14:13 UTC) #9
Message was sent while issue was closed.
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...

File chrome/content/cssProperties.js (right):

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?
> > > 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
> 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

File lib/filterClasses.js (right):

lib/filterClasses.js:937: let hasSelectorUsed =
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
> 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.

Powered by Google App Engine
This is Rietveld