Move scroll paint property nodes to be owned by the transform tree

Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

The major changes in this patch are:
* m_scroll has been removed from ObjectPaintProperties, as ownership is
now on the transform tree.
* m_scroll has been removed from FrameView, as ownership is now on the
transform tree.
* PaintLayer::sticksToViewport required some involved refactoring
because the cached scroll node is no longer available.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes.
This patch also passes 30 new layout tests with spv2 enabled.

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

Review-Url: https://codereview.chromium.org/2657863004
Cr-Commit-Position: refs/heads/master@{#447079}
Committed: https://chromium.googlesource.com/chromium/src/+/3eee970eb6757c6ea3997e0722d7bab727b9c11c

[pdr., 2017-01-26 06:11:13]

Description was changed from

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes

BUG=683774
==========

to

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes

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

[pdr., 2017-01-26 06:13:34]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wkorman@chromium.org

[pdr., 2017-01-26 20:04:14]

pdr@chromium.org changed reviewers:
+ trchen@chromium.org

[pdr., 2017-01-26 20:04:54]

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:351:
nearestScrollNode(layoutObject()
This part of the change is not entirely trivial and is a downside to the new
approach.

[wkorman, 2017-01-26 21:01:10]

lgtm

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:351:
nearestScrollNode(layoutObject()
Do existing tests cover this logic sufficiently or is it worth adding new
specifically for this?

[chrishtr, 2017-01-26 21:22:59]

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (left):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FramePainter.cpp:109:
properties.propertyTreeState.setScroll(scrollBarScroll);
Did this functionality get replaced?

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp:232: nullptr,
Not sentinel scroll root?

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:336: return
nearestScrollNode(transform->parent());
Please make this an iteration instead of recursion, to avoid stack overflows.

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:161:
TransformationMatrix frameScroll;
Move this down below 173 if it's not needed?

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
(left):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:102:
effect()->toTreeString() + "\n" + scroll()->toTreeString();
Still useful to print the scroll independently?

[pdr., 2017-01-27 20:09:33]

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (left):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FramePainter.cpp:109:
properties.propertyTreeState.setScroll(scrollBarScroll);
On 2017/01/26 at 21:22:58, chrishtr wrote:
> Did this functionality get replaced?

This shouldn't have been used. It was for painting scrollbars which don't need
scroll nodes.

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp:232: nullptr,
On 2017/01/26 at 21:22:58, chrishtr wrote:
> Not sentinel scroll root?

This is really just an overzealous check that the background doesn't scroll, but
it looks confusing. I've added a new function to TransformPaintPropertyNode to
make this clearer (isScrollTranslation()) and used the new function to make this
test clearer.

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:336: return
nearestScrollNode(transform->parent());
On 2017/01/26 at 21:22:58, chrishtr wrote:
> Please make this an iteration instead of recursion, to avoid stack overflows.

Done.

I found a bug fixing this: the root transform node referenced a null scroll node
which allowed for the root of the scroll tree to be null if no scrolling
occurred. This will be fine when we remove the non-null root concept, but is
wrong now. I fixed this by making the root transform node reference the root
scroll node. This is tested in ScrollsWithViewportFixedPosition.

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:351:
nearestScrollNode(layoutObject()
On 2017/01/26 at 21:01:10, wkorman wrote:
> Do existing tests cover this logic sufficiently or is it worth adding new
specifically for this?

I think we have pretty good coverage here. I wanted to fix this in a followup to
keep this patch simple, but found there were too many test failures. Some
examples:
PaintLayerTest.ScrollsWithViewportFixedPosition
PaintLayerTest.ScrollsWithViewportFixedPositionInsideTransform
PaintLayerTest.ScrollsWithViewportFixedPositionInsideTransformNoScroll

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:161:
TransformationMatrix frameScroll;
On 2017/01/26 at 21:22:58, chrishtr wrote:
> Move this down below 173 if it's not needed?

My goal in moving this is to initialize each of these variables in the order
that they are used in updateScrollTranslation on line 175.

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
(left):

https://codereview.chromium.org/2657863004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:102:
effect()->toTreeString() + "\n" + scroll()->toTreeString();
On 2017/01/26 at 21:22:58, chrishtr wrote:
> Still useful to print the scroll independently?

The scroll tree is now printed via the transform->toTreeString. The transform
nodes are now printed with scroll info if it exists, or without scroll info
otherwise.

The output looks like:
root 0x1c3c39d9c010 parent=0x0 transform=identity ... scroll=... clip=0x0
bounds=0x0 userScrollableHorizontal=no userScrollableVertical=no ...
  PreTranslation (FrameView) 0x1c3c39d9c100 parent=0x1c3c39d9c010
transform=identity ...

(I'll see if we can make this less verbose by removing some scroll printing in
the default case)

[pdr., 2017-01-27 20:09:43]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 20:10:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-01-27 21:10:49]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 21:11:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-01-27 22:22:45]

Description was changed from

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes

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

to

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

The major changes in this patch are:
* m_scroll has been removed from ObjectPaintProperties, as ownership is
now on the transform tree.
* m_scroll has been removed from FrameView, as ownership is now on the
transform tree.
* PaintLayer::sticksToViewport required some involved refactoring
because the cached scroll node is no longer available.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes.
This patch also passes 30 new layout tests with spv2 enabled.

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

[commit-bot: I haz the power, 2017-01-27 22:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:52:17]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-01-30 17:34:38]

PTAL

[wkorman, 2017-01-30 19:18:56]

lgtm

https://codereview.chromium.org/2657863004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2657863004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:70:
(ScrollPaintPropertyNode::create(
Looks like one too many parens around this?

[pdr., 2017-01-30 19:33:26]

https://codereview.chromium.org/2657863004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2657863004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:70:
(ScrollPaintPropertyNode::create(
On 2017/01/30 at 19:18:56, wkorman wrote:
> Looks like one too many parens around this?

Good catch, fixed.

[pdr., 2017-01-30 19:33:30]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 19:33:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-01-30 19:59:17]

lgtm

[pdr., 2017-01-30 19:59:32]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2017-01-30 19:59:42]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-01-30 19:59:43]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2657863004/#ps60001
(title: "Rebase & remove parens")

[commit-bot: I haz the power, 2017-01-30 19:59:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 21:50:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485806382636260,
"parent_rev": "f77ebec0c7db165ee6039e617432faa12c436641", "commit_rev":
"3eee970eb6757c6ea3997e0722d7bab727b9c11c"}

[commit-bot: I haz the power, 2017-01-30 21:51:31]

Description was changed from

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

The major changes in this patch are:
* m_scroll has been removed from ObjectPaintProperties, as ownership is
now on the transform tree.
* m_scroll has been removed from FrameView, as ownership is now on the
transform tree.
* PaintLayer::sticksToViewport required some involved refactoring
because the cached scroll node is no longer available.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes.
This patch also passes 30 new layout tests with spv2 enabled.

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

to

==========
Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

The major changes in this patch are:
* m_scroll has been removed from ObjectPaintProperties, as ownership is
now on the transform tree.
* m_scroll has been removed from FrameView, as ownership is now on the
transform tree.
* PaintLayer::sticksToViewport required some involved refactoring
because the cached scroll node is no longer available.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes.
This patch also passes 30 new layout tests with spv2 enabled.

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

Review-Url: https://codereview.chromium.org/2657863004
Cr-Commit-Position: refs/heads/master@{#447079}
Committed:
https://chromium.googlesource.com/chromium/src/+/3eee970eb6757c6ea3997e0722d7...
==========

[commit-bot: I haz the power, 2017-01-30 21:51:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3eee970eb6757c6ea3997e0722d7...