Issue 29585589: Issue 6423 - Observe only relevant mutation types (Closed)
|
Created:
Oct. 22, 2017, 10:51 p.m. by Manish Jethani Modified:
Feb. 28, 2018, 10:42 a.m. 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 #MessagesTotal messages: 11
Patch Set 1 We don't have to observe all mutation types all the time. https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHid... lib/content/elemHideEmulation.js:315: /\[.+\]/.test(selector._selector))|| This check for attribute selector syntax can give false positives in very rare cases, but it doesn't matter since this is just an optimization anyway. https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHid... lib/content/elemHideEmulation.js:316: (pattern.selectors[0] instanceof HasSelector && I realize that the second check is essentially pattern.selectors[0].dependsOnStyle. I think this logic generally should be moved into the respective classes.
Patch Set 2: Account for nested selectors The first one clearly was wrong and did not cover nested selectors. I've also cleaned it up now. https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585590/lib/content/elemHid... lib/content/elemHideEmulation.js:316: (pattern.selectors[0] instanceof HasSelector && On 2017/10/22 22:58:13, Manish Jethani wrote: > I realize that the second check is essentially > pattern.selectors[0].dependsOnStyle. I think this logic generally should be > moved into the respective classes. Done.
https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHid... lib/content/elemHideEmulation.js:149: this.maybeDependsOnAttributes = /\[.+\]/.test(selector); What about the changes of `class` and `id` ?
Patch Set 3: Account for ID and class selectors https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29585592/lib/content/elemHid... lib/content/elemHideEmulation.js:149: this.maybeDependsOnAttributes = /\[.+\]/.test(selector); On 2017/10/23 13:18:55, hub wrote: > What about the changes of `class` and `id` ? Done.
What about this one, does it look OK?
https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHid... lib/content/elemHideEmulation.js:332: selector.dependsOnStyles I'm always forgetting operator precedence. Should there be braces here to show explicitly the evaluation order? (I would put them)
Patch Set 5: Add brackets https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29585589/diff/29586713/lib/content/elemHid... lib/content/elemHideEmulation.js:332: selector.dependsOnStyles On 2018/02/21 18:12:27, hub wrote: > I'm always forgetting operator precedence. Should there be braces here to show > explicitly the evaluation order? (I would put them) Done.
I have filed a Trac issue now: https://issues.adblockplus.org/ticket/6423
as it is, LGTM But please get a signoff from the module owner.
Hubert knows this code better than me, if he's happy please go ahead. 
|
