Main thread scrolling reasons are broken with BlinkGenPropertyTrees |
||
Issue descriptionMain thread scrolling reasons currently live on both cc::Layer and cc::ScrollNode. Pre-BGPT, these would be computed by ScrollingCoordinator, set on cc::Layer, then synced to the property tree nodes which were built from cc:Layers. With BGPT, blink computes main thread scrolling reasons in the prepaint tree walk, then pushes main thread scrolling reasons to cc::ScrollNode in PaintArtifactCompositor. The reasons sent via BGPT are missing reasons such as slow viewport-attachments that need main-thread scrolling. We need to: 1) Move main_thread_scrolling_reasons off cc::Layer so ScrollNode::main_thread_scrolling_reasons is the source of truth. 2) Compute main_thread_scrolling_reasons correctly. Load the attached testcsae with these flags: --disable-prefer-compositing-to-lcd-text --enable-blink-features=BlinkGenPropertyTrees When scrolling, notice the pink rectangle flickers. This was not caught by tests because the tests read values off cc::Layer which are set correctly in BGPT via ScrollingCoordinator.
,
Dec 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d commit 1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d Author: Philip Rogers <pdr@chromium.org> Date: Mon Dec 17 22:30:31 2018 Remove CompositingTriggers CompositingTrigger had more of a purpose in the past, but is now only an alias of Settings::GetPreferCompositingToLCDTextEnabled(). Removing this simplifies the code and will let us make CompositingReasonFinder a static class in the future. Bug: 915372 Change-Id: I0de4ea30dbc5fe5278e8ae9f2e97d00ad6c734a2 Reviewed-on: https://chromium-review.googlesource.com/c/1380335 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#617260} [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/BUILD.gn [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.h [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.cc [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.h [delete] https://crrev.com/39448a3db140a19d21a1bbc7ce2739fefad914b5/third_party/blink/renderer/core/paint/compositing/compositing_triggers.h [modify] https://crrev.com/1e2d478cdaefd3e6425b64a3749910e9b4bb9d8d/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/234c126b9f939b2b7d491ac4189fcc6ba3f1604f commit 234c126b9f939b2b7d491ac4189fcc6ba3f1604f Author: Philip Rogers <pdr@chromium.org> Date: Tue Dec 18 19:57:27 2018 Make CompositingReasonFinder a static class This patch makes CompositingReasonFinder a static class (all functions static) because there is no state that needs to be tracked. This is just a simplification of the code and should not have any functional change. Bug: 915372 Change-Id: I39242f8de43705e3ccc61ae2ff9bb10d410b1823 Reviewed-on: https://chromium-review.googlesource.com/c/1381259 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#617597} [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_inputs_updater.cc [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_inputs_updater.h [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.h [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.cc [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/compositing_requirements_updater.h [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc [modify] https://crrev.com/234c126b9f939b2b7d491ac4189fcc6ba3f1604f/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f308c6727540f45873ee03e6f83384d6262a5583 commit f308c6727540f45873ee03e6f83384d6262a5583 Author: Philip Rogers <pdr@chromium.org> Date: Tue Dec 18 21:55:06 2018 Move main thread scrolling reasons off cc::Layer This patch de-duplicates the main thread scrolling reasons on cc::Layer, cc::LayerImpl, and cc::ScrollNode, making cc::ScrollNode's main thread scrolling reasons the source of truth. The major changes in this patch are: 1) cc::LayerImpl now uses the main thread scrolling reasons on the associated cc::ScrollNode. With BlinkGenPropertyTrees (BGPT), blink will generate the correct main thread scrolling reasons in the scroll tree and set them on the cc::ScrollNodes. Pre-BlinkGenPropertyTrees, blink will set main thread scrolling reasons on cc::Layer which will then be used by cc's property tree builder to set the value on cc::ScrollNodes. 2) Blink's property tree builder now correctly calculates main thread scrolling reasons. A TODO has been added for composite-after-paint. 3) When BGPT is enabled, ScrollingCoordinator is no longer used to update main thread scrolling reasons. 4) The ScrollingCoordinator tests have been parameterized to work with BGPT. A TODO has been added to move these tests out of the scrolling coordinator test file. Bug: 915372 Change-Id: Ib3ca1cf19fbfed3d04629d9229a1ce684214f33b Reviewed-on: https://chromium-review.googlesource.com/c/1369187 Reviewed-by: vmpstr <vmpstr@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#617642} [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer.h [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer_impl.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer_impl.h [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer_impl_test_properties.h [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/layers/layer_unittest.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/trees/layer_tree_host_common_unittest.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/cc/trees/property_tree_builder.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/frame/local_frame_view.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/frame/visual_viewport.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc [modify] https://crrev.com/f308c6727540f45873ee03e6f83384d6262a5583/third_party/blink/renderer/core/scroll/scrollable_area.cc
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0703d7bf820ce4b958e6f7f3cc12ee0b6a6e851 commit b0703d7bf820ce4b958e6f7f3cc12ee0b6a6e851 Author: Philip Rogers <pdr@chromium.org> Date: Wed Dec 19 17:54:28 2018 Split out MainThreadScrollingReasonsTest from ScrollingCoordinatorTest Main thread scrolling reasons are calculated in two places: 1) Pre-BlinkGenPropertyTrees (BGPT) in ScrollingCoordinator. 2) With BGPT, in PaintPropertyTreeBuilder. This patch extracts out the unit tests for main thread scrolling reasons into a new file. The new tests only run with BGPT on/off (the PaintTouchActionRects parameter does not affect main thread scrolling reasons). A few tests (e.g., FastScrollingForStickyPosition) have been partially duplicated because the ScrollingCoordinatorTest version needs to be kept for ScrollingCoordinator coverage. Some tests have been simplified by using GetViewMainThreadScrollingReasons. Bug: 915372 Change-Id: I0f928694cb6170cc3a3c95a1fd615ce671eacd7c Reviewed-on: https://chromium-review.googlesource.com/c/1383392 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#617874} [modify] https://crrev.com/b0703d7bf820ce4b958e6f7f3cc12ee0b6a6e851/third_party/blink/renderer/core/BUILD.gn [add] https://crrev.com/b0703d7bf820ce4b958e6f7f3cc12ee0b6a6e851/third_party/blink/renderer/core/page/scrolling/main_thread_scrolling_reasons_test.cc [modify] https://crrev.com/b0703d7bf820ce4b958e6f7f3cc12ee0b6a6e851/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
,
Dec 19
Woohoo!
,
Dec 20
Fantastic! Thank you!
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ea3826e22bdda4e42ba818869cf643f271669d2 commit 0ea3826e22bdda4e42ba818869cf643f271669d2 Author: Philip Rogers <pdr@chromium.org> Date: Thu Jan 10 02:35:09 2019 [BlinkGenPropertyTrees] Do not set main thread reasons for overlays This patch stops calling cc::Layer::AddMainThreadScrollingReasons for overlays when using layer lists (aka BlinkGenPropertyTrees) because these reasons need to be set on ScrollNodes. Because the frame overlay uses the root scroll node, no main thread reason is actually needed because it will not scroll. This is the penultimate patch towards removing layer-list-mode callers of Layer::AddMainThreadScrollingReasons. https://crbug.com/919969 has been filed to fix the one remaining issue and a commented-out TODO has been added in this patch to DCHECK that Layer::AddMainThreadScrollingReasons is not called in layer lists mode. Bug: 915372 , 919969 Change-Id: I31ffbb452b843fe7f498e23b1cffe429eb682a0c Reviewed-on: https://chromium-review.googlesource.com/c/1401277 Commit-Queue: Philip Rogers <pdr@chromium.org> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Reviewed-by: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#621430} [modify] https://crrev.com/0ea3826e22bdda4e42ba818869cf643f271669d2/cc/layers/layer.cc [modify] https://crrev.com/0ea3826e22bdda4e42ba818869cf643f271669d2/third_party/blink/renderer/core/frame/frame_overlay.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Dec 17