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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by Manish Jethani
Modified:
3 days, 3 hours ago
Reviewers:
hub
CC:
kzar, 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 #

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 0 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: 19
Manish Jethani
2 months, 2 weeks ago (2018-03-05 18:05:31 UTC) #1
Manish Jethani
Patch Set 1
2 months, 2 weeks ago (2018-03-05 18:16:15 UTC) #2
Manish Jethani
Patch Set 2: Rebase
2 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 ...
2 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 ...
1 month, 4 weeks 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))" ...
1 week, 2 days 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 ...
1 week, 2 days ago (2018-05-11 16:32:01 UTC) #7
Manish Jethani
Patch Set 5: Remove comment about removed nodes
1 week, 2 days 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 ...
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 ...
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 ...
6 days, 10 hours 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 ...
5 days, 18 hours 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 ...
5 days, 13 hours 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: > > > ...
5 days, 3 hours 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 days, 23 hours 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 days, 23 hours 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' ...
3 days, 10 hours 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 ...
3 days, 6 hours ago (2018-05-18 00:32:39 UTC) #18
Manish Jethani
3 days, 3 hours ago (2018-05-18 03:44:02 UTC) #19
Patch Set 8: Ignore own DOM modifications in debug mode
Patch Set 9: Rename debug mode to test mode

The problem was that the version of Firefox in the tests was observing
modifications to attribute values, even though we set "attributes: false". It's
better to just ignore any such mutations for the purpose of testing, and that's
what the updated patch does.
Sign in to reply to this message.

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