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

Issue 29714601: Issue 6437 - Skip elements not affected by DOM mutations (Closed)

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

Description

This builds on https://codereview.adblockplus.org/29712655/ Let's take the following document: <div id="n1"> <p id="n1_1"></p> <p id="n1_2"></p> <p id="n1_4">Hello</p> </div> <div id="n2"> <p id="n2_1"></p> <p id="n2_2"></p> <p id="n2_4">Hello</p> </div> <div id="n3"> <p id="n3_1"></p> <p id="n3_2"></p> <p id="n3_4">Hello</p> </div> <div id="n4"> <p id="n4_1"></p> <p id="n4_2"></p> <p id="n4_4">Hello</p> </div> Suppose we have the filter p:-abp-contains(Hello) When a new node n2_3 is added to the document, we check every element that matches the selector "p". This is wasteful and unnecessary. MutationObserver already tells us which nodes have been added, then we only need to check any of the new nodes' ancestors or descendants that match the selector. When we come across n1_1, for example, we can skip it because it obviously is not affected by the given set of mutations. The problem is even worse when you consider -abp-has and multiple levels of nesting.

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 1

Patch Set 3 : Rebase #

Patch Set 4 : Disable optimization for patterns containing sibling combinators #

Total comments: 1

Patch Set 5 : Remove comment about removed nodes #

Patch Set 6 : Ignore mutation targets when using style sheets #

Patch Set 7 : Add tests #

Total comments: 1

Patch Set 8 : Ignore own DOM modifications in debug mode #

Patch Set 9 : Rename debug mode to test mode #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -17 lines) Patch
M lib/content/elemHideEmulation.js View 1 2 3 4 5 6 7 8 11 chunks +125 lines, -13 lines 4 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 7 8 4 chunks +128 lines, -4 lines 0 comments Download

Messages

Total messages: 26
Manish Jethani
6 months, 2 weeks ago (2018-03-05 18:05:31 UTC) #1
Manish Jethani
Patch Set 1
6 months, 2 weeks ago (2018-03-05 18:16:15 UTC) #2
Manish Jethani
Patch Set 2: Rebase
6 months ago (2018-03-20 10:08:54 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29714601/diff/29728577/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29728577/lib/content/elemHideEmulation.js#newcode349 lib/content/elemHideEmulation.js:349: if (targets && !targets.some(target => element.contains(target) || By the ...
6 months ago (2018-03-20 10:21:57 UTC) #4
Manish Jethani
I just realized that this patch does not take into account sibling selectors (~ and ...
6 months ago (2018-03-22 16:06:10 UTC) #5
Manish Jethani
Patch Set 4: Disable optimization for patterns containing sibling combinators The pattern "#?#div.bar ~ div:-abp-has(>p:-abp-contains(Hello))" ...
4 months, 1 week ago (2018-05-11 16:28:35 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29714601/diff/29778597/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29778597/lib/content/elemHideEmulation.js#newcode542 lib/content/elemHideEmulation.js:542: // Ideally we would also be interested in removed ...
4 months, 1 week ago (2018-05-11 16:32:01 UTC) #7
Manish Jethani
Patch Set 5: Remove comment about removed nodes
4 months, 1 week ago (2018-05-11 16:47:57 UTC) #8
Manish Jethani
Note that this patch will break the following case on Firefox: <!DOCTYPE html> <article> Hello ...
4 months, 1 week ago (2018-05-13 14:42:35 UTC) #9
Manish Jethani
Patch Set 6: Ignore mutation targets when using style sheets On 2018/05/13 14:42:35, Manish Jethani ...
4 months, 1 week ago (2018-05-13 14:51:41 UTC) #10
hub
looking good, but I think we should try to have more test coverage in that ...
4 months, 1 week ago (2018-05-14 20:27:20 UTC) #11
Manish Jethani
On 2018/05/14 20:27:20, hub wrote: > looking good, but I think we should try to ...
4 months, 1 week ago (2018-05-15 12:44:09 UTC) #12
hub
On 2018/05/15 12:44:09, Manish Jethani wrote: > Do you mean we should test that unrelated ...
4 months, 1 week ago (2018-05-15 17:57:25 UTC) #13
Manish Jethani
On 2018/05/15 17:57:25, hub wrote: > On 2018/05/15 12:44:09, Manish Jethani wrote: > > > ...
4 months ago (2018-05-16 03:38:25 UTC) #14
Manish Jethani
Patch Set 7: Add tests On 2018/05/16 03:38:25, Manish Jethani wrote: > On 2018/05/15 17:57:25, ...
4 months ago (2018-05-16 07:21:54 UTC) #15
Manish Jethani
https://codereview.adblockplus.org/29714601/diff/29783555/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29783555/lib/content/elemHideEmulation.js#newcode32 lib/content/elemHideEmulation.js:32: lastProcessedElements: new Set() We could add another entry here, ...
4 months ago (2018-05-16 07:22:12 UTC) #16
hub
It seems that the test "testOnlyRelevantElementsProcessed" fails on Firefox AssertionError: The element with ID 'n1' ...
4 months ago (2018-05-17 20:35:10 UTC) #17
Manish Jethani
On 2018/05/17 20:35:10, hub wrote: > [...] > > I run the test applying this ...
4 months ago (2018-05-18 00:32:39 UTC) #18
Manish Jethani
Patch Set 8: Ignore own DOM modifications in debug mode Patch Set 9: Rename debug ...
4 months ago (2018-05-18 03:44:02 UTC) #19
hub
My comments on this are opening a can of worm concerning the testing frameworks we ...
3 months, 3 weeks ago (2018-05-24 19:19:10 UTC) #20
Manish Jethani
A couple of comments inline. So what do you think we should do now? We ...
3 months, 3 weeks ago (2018-05-25 07:21:25 UTC) #21
hub
if Dave is good with it, I'm good with it.
3 months, 3 weeks ago (2018-05-25 16:24:44 UTC) #22
Manish Jethani
On 2018/05/25 16:24:44, hub wrote: > if Dave is good with it, I'm good with ...
3 months, 3 weeks ago (2018-05-25 22:36:33 UTC) #23
Manish Jethani
On 2018/05/25 22:36:33, Manish Jethani wrote: > On 2018/05/25 16:24:44, hub wrote: > > if ...
2 months, 3 weeks ago (2018-06-25 16:36:24 UTC) #24
hub
LGTM. (I'll file a bug to improve the tests)
2 months, 3 weeks ago (2018-06-26 08:46:14 UTC) #25
hub
2 months, 3 weeks ago (2018-06-26 08:53:36 UTC) #26
Sign in to reply to this message.

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