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

Issue 29676761: Issue 6296 - Handle relative prefix in :-abp-has() (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by hub
Modified:
1 year, 5 months ago
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6296 - Handle relative prefix in :-abp-has()

Patch Set 1 #

Total comments: 7

Patch Set 2 : Rework the query selector all #

Total comments: 4

Patch Set 3 : addressed comments #

Total comments: 11

Patch Set 4 : No longer use regexp for relative selector. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -26 lines) Patch
M lib/content/elemHideEmulation.js View 1 2 3 4 chunks +67 lines, -25 lines 2 comments Download
M test/browser/elemHideEmulation.js View 2 chunks +29 lines, -1 line 0 comments Download

Messages

Total messages: 15
hub
1 year, 5 months ago (2018-01-22 21:20:57 UTC) #1
lainverse
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js#newcode201 lib/content/elemHideEmulation.js:201: let elements = subtree.querySelectorAll(actualPrefix); As I understand if this ...
1 year, 5 months ago (2018-01-23 01:02:51 UTC) #2
kzar
Would you be able to take a look at this one Manish?
1 year, 5 months ago (2018-01-23 10:13:00 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js#newcode197 lib/content/elemHideEmulation.js:197: if (relativeSelectorRegexp.test(actualPrefix)) If I understand this correctly, "+" and ...
1 year, 5 months ago (2018-01-23 16:42:11 UTC) #4
hub
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js#newcode201 lib/content/elemHideEmulation.js:201: let elements = subtree.querySelectorAll(actualPrefix); On 2018/01/23 16:42:11, Manish Jethani ...
1 year, 5 months ago (2018-01-23 16:51:11 UTC) #5
hub
https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29676762/lib/content/elemHideEmulation.js#newcode197 lib/content/elemHideEmulation.js:197: if (relativeSelectorRegexp.test(actualPrefix)) On 2018/01/23 16:42:11, Manish Jethani wrote: > ...
1 year, 5 months ago (2018-01-25 21:58:34 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHideEmulation.js#newcode155 lib/content/elemHideEmulation.js:155: if (scopeSupported) I think since the code in this ...
1 year, 5 months ago (2018-01-26 08:41:32 UTC) #7
hub
https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29679866/lib/content/elemHideEmulation.js#newcode155 lib/content/elemHideEmulation.js:155: if (scopeSupported) On 2018/01/26 08:41:32, Manish Jethani wrote: > ...
1 year, 5 months ago (2018-01-26 14:44:45 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js#newcode138 lib/content/elemHideEmulation.js:138: scopeSupported = false; Maybe we should add that comment ...
1 year, 5 months ago (2018-01-30 05:53:42 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js#newcode214 lib/content/elemHideEmulation.js:214: const relativeSelectorRegexp = /^>/; So I ran this on ...
1 year, 5 months ago (2018-01-30 06:13:07 UTC) #10
lainverse
I've modified your test in a way it likely to be used and it looks ...
1 year, 5 months ago (2018-01-30 14:27:33 UTC) #11
lainverse
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js#newcode148 lib/content/elemHideEmulation.js:148: * @returns {Node|NodeList} result of the query. null in ...
1 year, 5 months ago (2018-01-30 15:28:30 UTC) #12
hub
https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29680670/lib/content/elemHideEmulation.js#newcode138 lib/content/elemHideEmulation.js:138: scopeSupported = false; On 2018/01/30 05:53:41, Manish Jethani wrote: ...
1 year, 5 months ago (2018-01-30 16:03:58 UTC) #13
lainverse
https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHideEmulation.js#newcode214 lib/content/elemHideEmulation.js:214: const incompletePrefixRegexp = /[\s>+~]$/; Since that other regexp were ...
1 year, 5 months ago (2018-01-30 18:07:28 UTC) #14
Manish Jethani
1 year, 5 months ago (2018-01-30 20:23:20 UTC) #15
LGTM

https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid...
File lib/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29676761/diff/29684689/lib/content/elemHid...
lib/content/elemHideEmulation.js:214: const incompletePrefixRegexp = /[\s>+~]$/;
On 2018/01/30 18:07:28, lainverse wrote:
> Since that other regexp were replaced with string comparison it might be a
good
> idea to replace this one too:
> [snip]

If we do this then I think that it should be a separate patch.
Sign in to reply to this message.

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