Filterlist experiment #483
Conversation
There was a problem hiding this comment.
@muodov I think you're better off waiting for @sammacbeth's review before going ahead with this. It's a fairly large change and I'm not familiar with the codebase, I don't have time to get up to speed with the codebase enough to review the changes properly. That said I left a few comments, maybe they will help speed up the review when Sam's back?
| @@ -0,0 +1,3 @@ | |||
| @@||duckduckgo.com^$document | |||
There was a problem hiding this comment.
Shouldn't the $document filter completely allowlist duckduckgo.com (and subdomains)? Why are the other two filters necessary?
There was a problem hiding this comment.
The $document filter is not working for some reason. I think it might be a limitation of the adblocker library
There was a problem hiding this comment.
Oh right, that's weird. Might be worth leaving a comment if nothing else to explain that the filter parsing library you chose isn't working with $document allowing filters or something?
|
|
||
| export function getFilterlistSelectors(styles: string): string { | ||
| if (styles) { | ||
| const selectorsOnly = styles.replace(/\s*{[^\\}]*}\s*/g, ',').replace(/,$/, ''); |
There was a problem hiding this comment.
The idea is to strip out all {...} parts, so we have just the selectors without the actual CSS rules. Those selectors are then passed to a utility function that determines if there are any "visible" elements matching those.
There was a problem hiding this comment.
I wonder if it would be better to use the already parsed filters, to pull out the selector part. (Assuming the filter parsing library you used let's you do that.)
There was a problem hiding this comment.
Could we write a test for this to ensure that it's robust to changes in the output of the external adblocker library? i.e. a simple unit-test that creates a filter engine with dummy filters, gets the selectors and runs this function, emitting an expected list of selectors.
|
|
||
| export function getFilterlistSelectors(styles: string): string { | ||
| if (styles) { | ||
| const selectorsOnly = styles.replace(/\s*{[^\\}]*}\s*/g, ',').replace(/,$/, ''); |
There was a problem hiding this comment.
Could we write a test for this to ensure that it's robust to changes in the output of the external adblocker library? i.e. a simple unit-test that creates a filter engine with dummy filters, gets the selectors and runs this function, emitting an expected list of selectors.
https://app.asana.com/0/1201621853593513/1208032292432245/f