Bug 216355

Summary: Don't create event regions when the page has no subscrollers
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, pdr, simon.fraser, tonikitoo
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
simon.fraser: review+, koivisto: commit-queue+
patch none

Antti Koivisto
Reported 2020-09-10 05:27:36 PDT
Unless the page uses features like touch-action we don't need event regions for plain main frame scrolling.
Attachments
patch (50.43 KB, patch)
2020-09-10 06:14 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
patch (50.28 KB, patch)
2020-09-10 06:15 PDT, Antti Koivisto 		simon.fraser: review+
koivisto: commit-queue+
DetailsFormatted DiffDiff
patch (50.30 KB, patch)
2020-09-10 11:04 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Antti Koivisto
Comment 1 2020-09-10 06:05:37 PDT
<rdar://problem/67900642>
Antti Koivisto
Comment 2 2020-09-10 06:14:31 PDT
Created attachment 408431 [details] patch
Antti Koivisto
Comment 3 2020-09-10 06:15:10 PDT
Created attachment 408432 [details] patch
Simon Fraser (smfr)
Comment 4 2020-09-10 08:57:50 PDT
Comment on attachment 408432 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=408432&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:880 > + if (scrollingCoordinator && scrollingCoordinator->hasSubscrollers() != hadSubscrollers) > + invalidateEventRegionForAllFrames(); I don't like that this triggers a second compositing update just after we've finished one, and in all frames too! This means that every page with one or more subscrollers now suffers from an extra full layer tree walk in all frames and a second compositing update.
Antti Koivisto
Comment 5 2020-09-10 09:29:14 PDT
> I don't like that this triggers a second compositing update just after we've > finished one, and in all frames too! No it doesn't. It just sets m_needsEventRegionUpdate bit in RenderLayerBacking.
Simon Fraser (smfr)
Comment 6 2020-09-10 09:42:39 PDT
Comment on attachment 408432 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=408432&action=review > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:39 > + scrollingStateTree().scrollingNodeAdded(); I wonder if ScrollingStateTree::addNode() is a better place to call this. > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:80 > + scrollingStateTree().scrollingNodeRemoved(); I wonder if ScrollingStateTree::willRemoveNode() is a better place to call this. >> Source/WebCore/rendering/RenderLayerCompositor.cpp:880 >> + invalidateEventRegionForAllFrames(); > > I don't like that this triggers a second compositing update just after we've finished one, and in all frames too! > > This means that every page with one or more subscrollers now suffers from an extra full layer tree walk in all frames and a second compositing update. I see now that we'll update regions at the end of the rendering update.
Antti Koivisto
Comment 7 2020-09-10 10:09:22 PDT
> I wonder if ScrollingStateTree::addNode() is a better place to call this. > I wonder if ScrollingStateTree::willRemoveNode() is a better place to call > this. I considered it but this seemed like the most robust solution.
EWS
Comment 8 2020-09-10 10:40:39 PDT
commit-queue failed to commit attachment 408432 [details] to WebKit repository.
Antti Koivisto
Comment 9 2020-09-10 11:04:23 PDT
Created attachment 408461 [details] patch
EWS
Comment 10 2020-09-10 11:36:29 PDT
Committed r266846: <https://trac.webkit.org/changeset/266846> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408461 [details].
Note You need to log in before you can comment on or make changes to this bug.