New issue
Advanced search Search tips

Issue 664672 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Support property tree invalidation when main thread scrolling reasons change

Project Member Reported by pdr@chromium.org, Nov 11 2016

Issue description

See above.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3797c08066b335c81847266d1f16dafc9b21e886

commit 3797c08066b335c81847266d1f16dafc9b21e886
Author: pdr <pdr@chromium.org>
Date: Sat Nov 19 01:11:20 2016

Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for simplicity. FrameView already
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object. In the common case, the set is empty.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG= 664672 , 645667 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2506353002
Cr-Commit-Position: refs/heads/master@{#433367}

[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
[modify] https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h

Comment 2 by pdr@chromium.org, Nov 19 2016

Status: Fixed (was: Assigned)
See above.

Comment 3 by pdr@chromium.org, Dec 13 2016

Status: Assigned (was: Fixed)
I misunderstood how main thread scrolling reasons differ between document and non-document scrollers: http://jsbin.com/fuwiduv vs http://jsbin.com/fiheged.

Needs a spv2 fix.
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f8b383bba59a8240c646a99fb0a5b2afc995d5d

commit 0f8b383bba59a8240c646a99fb0a5b2afc995d5d
Author: pdr <pdr@chromium.org>
Date: Tue Jan 10 00:19:26 2017

Rewrite how paint properties are built with bg:fixed main thread scrolling

Background-attachment: fixed forces main thread scrolling. The initial
paint property implementation [1] incorrectly allowed subtrees to scroll
off the main thread when an ancestor forced main thread scrolling reasons.

If there are three nested frames (A->B->C) and B has bg: fixed objects,
both B and C should have main thread reasons set (in [1], A and B would).
This patch also removes support for non-frame scrollers to change main
thread scrolling reasons for bg: fixed so that spv1 and spv2 match.

[1] https://chromium.googlesource.com/chromium/src/+/3797c08066b335c81847266d1f16dafc9b21e886

BUG= 664672 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2619393002
Cr-Commit-Position: refs/heads/master@{#442413}

[modify] https://crrev.com/0f8b383bba59a8240c646a99fb0a5b2afc995d5d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/0f8b383bba59a8240c646a99fb0a5b2afc995d5d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/0f8b383bba59a8240c646a99fb0a5b2afc995d5d/third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp

Comment 5 by pdr@chromium.org, Jan 23 2017

Status: Fixed (was: Assigned)

Sign in to add a comment