Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(499)

Issue 29383960: Issue 3143 - Filter elements with :-abp-has() (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by hub
Modified:
1 week, 2 days ago
CC:
Felix Dahlke
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+736 lines, -134 lines) Patch
M chrome/content/.eslintrc.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/content/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +313 lines, -107 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +421 lines, -25 lines 0 comments Download

Messages

Total messages: 52
hub
3 months, 1 week ago (2017-03-14 23:20:19 UTC) #1
hub
Felix, This is my initial version of the implementation for feedback. if this way of ...
3 months, 1 week ago (2017-03-14 23:24:14 UTC) #2
hub
This update hide the elements directly instead of using styles. I think it is a ...
3 months, 1 week ago (2017-03-16 05:17:44 UTC) #3
hub
the test that currently fail is because I haven't finished implementing it. I'm more confident ...
3 months, 1 week ago (2017-03-17 01:28:27 UTC) #4
hub
Right now we only miss supporting :has(:has()). The library clients (add-ons) needs to be updated. ...
3 months ago (2017-03-22 01:47:07 UTC) #5
hub
:has(:has()) now work.
2 months, 3 weeks ago (2017-03-28 20:12:45 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elemHideEmulation.js#newcode13 chrome/content/elemHideEmulation.js:13: Element.prototype.mozMatchesSelector || These fallbacks seem unnecessary. We only support ...
2 months, 3 weeks ago (2017-03-29 07:46:19 UTC) #7
hub
will update patch. https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29396655/chrome/content/elemHideEmulation.js#newcode13 chrome/content/elemHideEmulation.js:13: Element.prototype.mozMatchesSelector || On 2017/03/29 07:46:19, Sebastian ...
2 months, 3 weeks ago (2017-03-29 14:00:00 UTC) #8
hub
Actually the implementation has a major problem. If I do ":has(div.aclass)" as expected it will ...
2 months, 3 weeks ago (2017-03-31 04:14:57 UTC) #9
hub
On 2017/03/31 04:14:57, hub wrote: > Actually the implementation has a major problem. > > ...
2 months, 3 weeks ago (2017-04-03 00:21:50 UTC) #10
hub
this patch is much better. it works in the chrome addon with my patch.
2 months, 2 weeks ago (2017-04-03 08:12:38 UTC) #11
Felix Dahlke
I only had a pretty high level look, but the approach looks good as far ...
2 months, 1 week ago (2017-04-10 10:55:13 UTC) #12
hub
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js#newcode98 chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 10:55:12, Felix Dahlke ...
2 months, 1 week ago (2017-04-10 12:29:27 UTC) #13
Felix Dahlke
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js#newcode98 chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 12:29:27, hub wrote: ...
2 months, 1 week ago (2017-04-10 12:35:42 UTC) #14
hub
https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408705/chrome/content/elemHideEmulation.js#newcode98 chrome/content/elemHideEmulation.js:98: var match = pseudoClassHasSelectorRegExp.exec(selector); On 2017/04/10 12:35:41, Felix Dahlke ...
2 months, 1 week ago (2017-04-10 12:41:02 UTC) #15
hub
Updated patch to take felix comments into account. And added tests for it.
2 months, 1 week ago (2017-04-10 13:22:09 UTC) #16
Wladimir Palant
I've only done a high-level review of the approach so far. There is rather significant ...
1 month, 4 weeks ago (2017-04-25 10:57:53 UTC) #17
Wladimir Palant
Sorry, I published my comments prematurely. I meant to write some more. First of all, ...
1 month, 4 weeks ago (2017-04-25 11:30:46 UTC) #18
hub
I'll rework the patch to implement these suggestions. https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elemHideEmulation.js#newcode2 chrome/content/elemHideEmulation.js:2: // ...
1 month, 4 weeks ago (2017-04-25 19:42:48 UTC) #19
Wladimir Palant
I realized that I oversimplified with my suggestions above - always running querySelectorAll() on the ...
1 month, 4 weeks ago (2017-04-26 00:03:09 UTC) #20
Wladimir Palant
On 2017/04/25 11:30:46, Wladimir Palant wrote: > However, I would like to avoid messing with ...
1 month, 3 weeks ago (2017-04-26 05:41:22 UTC) #21
Wladimir Palant
On 2017/04/26 05:41:22, Wladimir Palant wrote: > we found initially and add a mutation observer. ...
1 month, 3 weeks ago (2017-04-26 05:53:58 UTC) #22
hub
https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29408731/chrome/content/elemHideEmulation.js#newcode2 chrome/content/elemHideEmulation.js:2: // used in the browser tests. See https://issues.adblockplus.org/ticket/4796 On ...
1 month, 3 weeks ago (2017-04-27 17:43:04 UTC) #23
hub
This version is missing just a couple of things like a non-regexp based parser. But ...
1 month, 3 weeks ago (2017-05-03 01:44:11 UTC) #24
hub
I have updated the Chrome implementation it is now simpler. And still work.
1 month, 2 weeks ago (2017-05-04 20:35:20 UTC) #25
hub
And as a side note, it applies on today master with Wladimir changes to have ...
1 month, 2 weeks ago (2017-05-05 16:11:48 UTC) #26
hub
https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elemHideEmulation.js#newcode80 chrome/content/elemHideEmulation.js:80: if (node && node.id && node.id != "") I ...
1 month, 1 week ago (2017-05-12 17:31:19 UTC) #27
hub
https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29437577/chrome/content/elemHideEmulation.js#newcode162 chrome/content/elemHideEmulation.js:162: let suffix = parseSelector(selector.substr(suffixStart)); here, if we had been ...
1 month, 1 week ago (2017-05-12 19:48:24 UTC) #28
Wladimir Palant
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode61 chrome/content/elemHideEmulation.js:61: // 1 base index like for :nth-child() Nit: Here ...
1 month, 1 week ago (2017-05-16 13:50:41 UTC) #29
hub
I'll update the patch with what's left. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode64 chrome/content/elemHideEmulation.js:64: let parentNode ...
1 month, 1 week ago (2017-05-16 21:35:56 UTC) #30
hub
Just a few things still not resolved. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode61 chrome/content/elemHideEmulation.js:61: // 1 ...
4 weeks ago (2017-05-25 13:02:31 UTC) #31
hub
I need to address that part. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode264 chrome/content/elemHideEmulation.js:264: *getElements(prefix, subtree, stylesheet) ...
3 weeks, 6 days ago (2017-05-26 12:42:47 UTC) #32
hub
This is the status of the latest revision of the patch. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): ...
3 weeks, 2 days ago (2017-05-31 02:16:50 UTC) #33
Wladimir Palant
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode103 chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/05/16 21:35:53, hub wrote: ...
3 weeks ago (2017-06-01 10:16:40 UTC) #34
hub
Also re-tested in chrome. The chrome patch will need to be updated. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js ...
3 weeks ago (2017-06-01 18:23:01 UTC) #35
Wladimir Palant
https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29452284/chrome/content/elemHideEmulation.js#newcode208 chrome/content/elemHideEmulation.js:208: suffixStart = startIndex + content.end + 1; On 2017/06/01 ...
2 weeks, 1 day ago (2017-06-07 08:33:00 UTC) #36
Wladimir Palant
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode103 chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/01 18:22:54, hub wrote: ...
2 weeks, 1 day ago (2017-06-07 08:35:20 UTC) #37
hub
Updated patch https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode103 chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 08:35:19, ...
2 weeks, 1 day ago (2017-06-07 14:15:11 UTC) #38
Wladimir Palant
https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode103 chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 14:15:06, hub wrote: ...
2 weeks, 1 day ago (2017-06-07 14:53:43 UTC) #39
hub
Patch updated. https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29438559/chrome/content/elemHideEmulation.js#newcode103 chrome/content/elemHideEmulation.js:103: newSelector += "> "; On 2017/06/07 14:53:40, ...
2 weeks, 1 day ago (2017-06-07 15:08:11 UTC) #40
Wladimir Palant
I hope that you didn't overlook the other four comments. I'll wait for those to ...
2 weeks, 1 day ago (2017-06-07 19:43:54 UTC) #41
hub
You were right, I mistakenly overlooked this other comment (I totally skipped them). Now all ...
2 weeks, 1 day ago (2017-06-08 00:17:23 UTC) #42
Sebastian Noack
https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js#newcode152 chrome/content/elemHideEmulation.js:152: if (content == null) Nit: Given that parseSelectorContent() either ...
2 weeks ago (2017-06-08 08:28:49 UTC) #43
Wladimir Palant
I didn't finish going through the unit tests today, but I thought that I would ...
2 weeks ago (2017-06-08 13:12:44 UTC) #44
hub
addressed both Sebastian and Wladimir comments. https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js#newcode152 chrome/content/elemHideEmulation.js:152: if (content == ...
2 weeks ago (2017-06-08 15:45:53 UTC) #45
Felix Dahlke
https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29458644/chrome/content/elemHideEmulation.js#newcode154 chrome/content/elemHideEmulation.js:154: console.error(new SyntaxError("Failed parsing AdBlock Plus " + On 2017/06/08 ...
1 week, 6 days ago (2017-06-09 09:02:34 UTC) #46
hub
Browser environment is no longer expected https://codereview.adblockplus.org/29383960/diff/29459612/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29459612/chrome/content/elemHideEmulation.js#newcode378 chrome/content/elemHideEmulation.js:378: 0, "", document, ...
1 week, 3 days ago (2017-06-12 15:10:09 UTC) #47
Wladimir Palant
This is good to land if you move reportError assignment into getFiltersFunc callback (quick and ...
1 week, 2 days ago (2017-06-13 13:16:27 UTC) #48
hub
https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29383960/diff/29463575/chrome/content/elemHideEmulation.js#newcode331 chrome/content/elemHideEmulation.js:331: reportError = error => this.window.console.error(error); On 2017/06/13 13:16:25, Wladimir ...
1 week, 2 days ago (2017-06-13 13:53:34 UTC) #49
hub
> > I will accept 1) given the urgency but only if an issue is ...
1 week, 2 days ago (2017-06-13 14:16:40 UTC) #50
hub
felix gave me the go ahead to push this.
1 week, 2 days ago (2017-06-14 01:14:04 UTC) #51
hub
1 week, 2 days ago (2017-06-14 01:16:32 UTC) #52
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/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5