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

Issue 29585589: Issue 6423 - Observe only relevant mutation types (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by Manish Jethani
Modified:
2 years, 3 months ago
Reviewers:
hub, kzar
CC:
sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 3

Patch Set 2 : Account for nested selectors #

Total comments: 2

Patch Set 3 : Account for ID and class selectors #

Total comments: 2

Patch Set 4 : Rebase #

Patch Set 5 : Add brackets #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -2 lines) Patch
M lib/content/elemHideEmulation.js View 1 2 3 4 5 chunks +45 lines, -2 lines 0 comments Download

Messages

Total messages: 11
Manish Jethani
2 years, 7 months ago (2017-10-22 22:51:31 UTC) #1
Manish Jethani
Patch Set 1 We don't have to observe all mutation types all the time. https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHideEmulation.js ...
2 years, 7 months ago (2017-10-22 22:58:14 UTC) #2
Manish Jethani
Patch Set 2: Account for nested selectors The first one clearly was wrong and did ...
2 years, 7 months ago (2017-10-23 00:12:10 UTC) #3
hub
https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHideEmulation.js#newcode149 lib/content/elemHideEmulation.js:149: this.maybeDependsOnAttributes = /\[.+\]/.test(selector); What about the changes of `class` ...
2 years, 7 months ago (2017-10-23 13:18:56 UTC) #4
Manish Jethani
Patch Set 3: Account for ID and class selectors https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHideEmulation.js#newcode149 lib/content/elemHideEmulation.js:149: ...
2 years, 7 months ago (2017-10-23 22:52:08 UTC) #5
Manish Jethani
What about this one, does it look OK?
2 years, 3 months ago (2018-02-21 16:42:34 UTC) #6
hub
https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHideEmulation.js#newcode332 lib/content/elemHideEmulation.js:332: selector.dependsOnStyles I'm always forgetting operator precedence. Should there be ...
2 years, 3 months ago (2018-02-21 18:12:27 UTC) #7
Manish Jethani
Patch Set 5: Add brackets https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHideEmulation.js#newcode332 lib/content/elemHideEmulation.js:332: selector.dependsOnStyles On 2018/02/21 18:12:27, ...
2 years, 3 months ago (2018-02-22 07:14:33 UTC) #8
Manish Jethani
I have filed a Trac issue now: https://issues.adblockplus.org/ticket/6423
2 years, 3 months ago (2018-02-26 13:59:28 UTC) #9
hub
as it is, LGTM But please get a signoff from the module owner.
2 years, 3 months ago (2018-02-26 15:38:41 UTC) #10
kzar
2 years, 3 months ago (2018-02-26 17:19:51 UTC) #11
Hubert knows this code better than me, if he's happy please go ahead.
Sign in to reply to this message.

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