[SPV2] Implement the blink-side scroll property tree

[SPV2] Implement the blink-side scroll property tree

Blink's scroll tree stores auxiliary scrolling information for
compositor-thread scrolling. This includes the hierarchy of scrolling
(which areas scroll relative to which other areas), whether an area
is user-scrollable in the x or y axes, and the extent that an area
can be scrolled. This patch is an initial implementation of the
blink scroll paint property tree with documentation, a followup will
plumb this to the compositor.

A proof-of-concept for the end-to-end design is available at:
https://codereview.chromium.org/2299533002

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

Committed: https://crrev.com/d1a5fe1b572bf7b446d45558823ac7a8d759628c
Cr-Commit-Position: refs/heads/master@{#417850}

[pdr., 2016-09-07 06:17:39]

Description was changed from

==========
[SPV2] Implement the blink-side scroll property tree

Blink's scroll tree stores auxiliary scrolling information for
compositor-thread scrolling. This includes the hierarchy of scrolling
(which areas scroll relative to which other areas), whether an area
is user-scrollable in the x or y axes, and the extent that an area
can be scrolled. This patch is an initial implementation of the
blink scroll paint property tree with documentation, a followup will
plumb this to the compositor.

A proof-of-concept for the end-to-end design is available at:
https://codereview.chromium.org/2299533002

BUG=644514
==========

to

==========
[SPV2] Implement the blink-side scroll property tree

Blink's scroll tree stores auxiliary scrolling information for
compositor-thread scrolling. This includes the hierarchy of scrolling
(which areas scroll relative to which other areas), whether an area
is user-scrollable in the x or y axes, and the extent that an area
can be scrolled. This patch is an initial implementation of the
blink scroll paint property tree with documentation, a followup will
plumb this to the compositor.

A proof-of-concept for the end-to-end design is available at:
https://codereview.chromium.org/2299533002

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

[pdr., 2016-09-07 06:18:31]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org, chrishtr@chromium.org, trchen@chromium.org,
xidachen@chromium.org

[pdr., 2016-09-07 06:19:24]

pdr@chromium.org changed reviewers:
+ sunxd@chromium.org
- xidachen@chromium.org

[trchen, 2016-09-07 23:12:53]

Overall looks good to me. My comments here are just FYI instead of suggestions.

+cc skobes@
I think there will be weird interaction between RLS and the dual viewport
system. Frame scrolling applies to visual (pinch-zoom) viewport first then
propagate up to layout viewport if there is leftover (See RootFrameViewport).
It is out of scope of this CL. Just to keep him in sync and start discussion
early.

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:52:
// this is the area that could respond to user input.
Why is this needed in the scroll node? I think the "geometry property states"
are enough to do perform hit testing. Also a rect is not complete without a base
transform space.

I'm guessing is it used to determine the scroll extent?
i.e. scroll_extent = deconvolute(bounds, clip)

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:56:
const FloatSize& bounds() const { return m_bounds; }
Not sure if it should be a Rect or a Size.

There are two scroll coordinate systems in Blink. The one used by ScrollableArea
[minPos, maxPos], and a shifted one used by Scrollbar (and I think cc use it
too) [0, maxPos - minPos]. Most of the time they are the same as minPos would be
0, but with special writing-mode it may not be the case.

For example:
<div style="direction:rtl; width:400px; height:300px; overflow:scroll;">
    <div style="width:1000px; height:1000px;"></div>
</div>

The layout overflow rect would be (l=-600, t=0, r=400, b=1000) and the scroll
extent would be (l=-600, t=0, r=0, b=600).
----
Another thing I still haven't think through is whether this should be FloatRect
or LayoutRect or IntRect.

Traditionally DOM only supports integer boxes and integer scroll range/offsets.
For smooth scrolling on high-DPI devices we allow fractional scroll delta (that
snaps to device pixels), but the nominal scroll offset is still integer I think.

I think it probably shouldn't be LayoutRect. Layout units should be internal to
layout and we only allowed them to leak up to paint.

It also involves another dilemma that we delay snapping until painting, but
determining scrollableness and scroll extent needs to be done in layout.

Here is an example of the dilemma: http://jsbin.com/jetaza/

It can be seen that the bottom border of the red box is clipped by its parent,
but the parent didn't create a scrollbar despite that it has overflow:auto.
(Even with scroll-y:scroll, the scrollbar is still disabled.)

[skobes, 2016-09-07 23:21:56]

On 2016/09/07 23:12:53, trchen wrote:
> +cc skobes@
> I think there will be weird interaction between RLS and the dual viewport
> system. Frame scrolling applies to visual (pinch-zoom) viewport first then
> propagate up to layout viewport if there is leftover (See RootFrameViewport).
> It is out of scope of this CL. Just to keep him in sync and start discussion
> early.

There might be some weirdness left to iron out, but I think the core
dual-viewport logic works with RLS.  RootFrameViewport::m_layoutViewport will
point to the LayoutView's PLSA.

[pdr., 2016-09-08 02:29:42]

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:52:
// this is the area that could respond to user input.
On 2016/09/07 at 23:12:53, trchen wrote:
> Why is this needed in the scroll node? I think the "geometry property states"
are enough to do perform hit testing. Also a rect is not complete without a base
transform space.
> 
> I'm guessing is it used to determine the scroll extent?
> i.e. scroll_extent = deconvolute(bounds, clip)

That's a good point. I think we should remove the hit testing part of this
comment and just refer to the scrolling clip extents aspect.

I've updated this comment in the latest patch.

https://codereview.chromium.org/2320463002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:56:
const FloatSize& bounds() const { return m_bounds; }
On 2016/09/07 at 23:12:53, trchen wrote:
> Not sure if it should be a Rect or a Size.
> 
> There are two scroll coordinate systems in Blink. The one used by
ScrollableArea [minPos, maxPos], and a shifted one used by Scrollbar (and I
think cc use it too) [0, maxPos - minPos]. Most of the time they are the same as
minPos would be 0, but with special writing-mode it may not be the case.

+1, you've thought through this! cc's ScrollNode does have an offset
("offset_to_transform_parent") that essentially makes this a rect instead of a
size.

I haven't prototyped this, but my high-level thinking is that we can avoid the
complexity of needing an offset by creating a transform node for the scrollbars.
This would also come in handy for geometry mapper to handle hit testing of
scrollbars. Thoughts?

> 
> For example:
> <div style="direction:rtl; width:400px; height:300px; overflow:scroll;">
>     <div style="width:1000px; height:1000px;"></div>
> </div>
> 
> The layout overflow rect would be (l=-600, t=0, r=400, b=1000) and the scroll
extent would be (l=-600, t=0, r=0, b=600).
> ----
> Another thing I still haven't think through is whether this should be
FloatRect or LayoutRect or IntRect.
> 
> Traditionally DOM only supports integer boxes and integer scroll
range/offsets. For smooth scrolling on high-DPI devices we allow fractional
scroll delta (that snaps to device pixels), but the nominal scroll offset is
still integer I think.
> 
> I think it probably shouldn't be LayoutRect. Layout units should be internal
to layout and we only allowed them to leak up to paint.
> 
> It also involves another dilemma that we delay snapping until painting, but
determining scrollableness and scroll extent needs to be done in layout.
> 
> Here is an example of the dilemma: http://jsbin.com/jetaza/
> 
> It can be seen that the bottom border of the red box is clipped by its parent,
but the parent didn't create a scrollbar despite that it has overflow:auto.
(Even with scroll-y:scroll, the scrollbar is still disabled.)

Excellent points! For the initial release of SPV2 I think we should strive to
match existing behavior. This approach makes a lot of stuff easier, such as not
needing two sets of expected layout tests. It's clear there is room for
improvement here though.

In the latest patch I've switched to using an IntSize which matches both the
inputs from the layout tree and the outputs in cc's ScrollNode.

[chrishtr, 2016-09-08 20:43:59]

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:89: #include
"core/paint/PaintPropertyTreePrinter.h"
remove

I think it would be reasonable to #include this from ObjectPaintProperties.h
though, to save work
debuggging future patches.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
Comments explaining the difference between scroll and root scroll?

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: // Auxiliary
scrolling information for compositor-thread scrolling. Includes information such
as the
Remove compositor-thread reference. It's for scrolling, full stop. In the future
the hope is to
use it for main-thread scrolling also.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:61: const
ScrollPaintPropertyNode* scroll() const { return m_scroll.get(); }
Also note that this data corresponds to the scroll implemented by
m_scrollTranslation.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:92: if
(ScrollPaintPropertyNode* existingScroll = frameView.scroll())
Maybe add frameview.createOrUpdateScroll() to avoid this conditional?

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:47: // The
scroll node contains information for compositor-thread scrolling such as the
remove compositor-thread

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1853: " 
#positionedOverflow {"
s/positionedOverflow/absposOverflow/

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1866: " 
<div id='positionedOverflow'>"
Also add a fixed-pos child. And another test that has an overflow:scroll element
that has position:absolute

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:38: //
SVGImageForContainer contains a reference to an Image (SVGImage*) and
Why add these comments in this patch?

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/README.md:98: Each paint chunk
is associated with a [scroll node](ScrollPaintPropertyNode.h)
Always? Even for a page that has no scrolling?

[pdr., 2016-09-08 23:16:26]

PTAL

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:89: #include
"core/paint/PaintPropertyTreePrinter.h"
On 2016/09/08 at 20:43:59, chrishtr wrote:
> remove
> 
> I think it would be reasonable to #include this from ObjectPaintProperties.h
though, to save work
> debuggging future patches.

Oops, good catch.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Comments explaining the difference between scroll and root scroll?

The root nodes on FrameView just store no-op paint properties to match cc's
trees and to avoid using null for the root. This looks pretty silly for the
scroll tree.

I'd like to revisit the non-null root property tree node discussion...

Because we already differ from cc's trees at the root nodes (e.g., no viewport
transform yet), I think we should actually remove the no-op root nodes after all
and use null for the root of all trees. This would avoid needing to add a
scrollable=false bit just for the root scroll node. I could do this as a patch
before this patch. WDYT?

The primary downside to removing no-op root nodes is having to tell szager he
was right.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:59: // Auxiliary
scrolling information for compositor-thread scrolling. Includes information such
as the
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Remove compositor-thread reference. It's for scrolling, full stop. In the
future the hope is to
> use it for main-thread scrolling also.

Done.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:61: const
ScrollPaintPropertyNode* scroll() const { return m_scroll.get(); }
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Also note that this data corresponds to the scroll implemented by
m_scrollTranslation.

Done.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:92: if
(ScrollPaintPropertyNode* existingScroll = frameView.scroll())
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Maybe add frameview.createOrUpdateScroll() to avoid this conditional?

I'd like to avoid putting property tree logic on Layout classes, sort of like
how put paint logic in separate painter classes. For LayoutObject we use
ObjectPaintProperties, but we've been managing FrameView's sloppily in this
function.

How does breaking these createOrUpdates out into helpers in
PaintPropertyTreeBuilder sound to you? In the latest patch I moved the logic
into 4 new functions:
createOrUpdateFrameViewPreTranslation
createOrUpdateFrameViewContentClip
createOrUpdateFrameViewScrollTranslation
createOrUpdateFrameViewScroll

(BTW, all of this will go away with RootLayerScrolls)

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:47: // The
scroll node contains information for compositor-thread scrolling such as the
On 2016/09/08 at 20:43:59, chrishtr wrote:
> remove compositor-thread

Done

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1866: " 
<div id='positionedOverflow'>"
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Also add a fixed-pos child. And another test that has an overflow:scroll
element that has position:absolute

Adding the fixed-pos test revealed a bug where we were creating a scroll node
for all FrameViews but only need to create them if frameView.isScrollable is
true. Fixed the bug and added the test.

I think this test covers the overflow:scroll element with position absolute.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:38: //
SVGImageForContainer contains a reference to an Image (SVGImage*) and
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Why add these comments in this patch?

Oops, this was from https://codereview.chromium.org/2322803002. Removed.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/README.md:98: Each paint chunk
is associated with a [scroll node](ScrollPaintPropertyNode.h)
On 2016/09/08 at 20:43:59, chrishtr wrote:
> Always? Even for a page that has no scrolling?

This is due to the no-op scroll node we add at the root.

[chrishtr, 2016-09-09 16:27:28]

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
On 2016/09/08 at 23:16:26, pdr. wrote:
> On 2016/09/08 at 20:43:59, chrishtr wrote:
> > Comments explaining the difference between scroll and root scroll?
> 
> The root nodes on FrameView just store no-op paint properties to match cc's
trees and to avoid using null for the root. This looks pretty silly for the
scroll tree.
> 
> I'd like to revisit the non-null root property tree node discussion...
> 
> Because we already differ from cc's trees at the root nodes (e.g., no viewport
transform yet), I think we should actually remove the no-op root nodes after all
and use null for the root of all trees. This would avoid needing to add a
scrollable=false bit just for the root scroll node. I could do this as a patch
before this patch. WDYT?
> 
> The primary downside to removing no-op root nodes is having to tell szager he
was right.

If you want to do that, I'd suggest after this patch rather than before. Forward
progess.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:92: if
(ScrollPaintPropertyNode* existingScroll = frameView.scroll())
On 2016/09/08 at 23:16:26, pdr. wrote:
> On 2016/09/08 at 20:43:59, chrishtr wrote:
> > Maybe add frameview.createOrUpdateScroll() to avoid this conditional?
> 
> I'd like to avoid putting property tree logic on Layout classes, sort of like
how put paint logic in separate painter classes. For LayoutObject we use
ObjectPaintProperties, but we've been managing FrameView's sloppily in this
function.
> 
> How does breaking these createOrUpdates out into helpers in
PaintPropertyTreeBuilder sound to you? In the latest patch I moved the logic
into 4 new functions:
> createOrUpdateFrameViewPreTranslation
> createOrUpdateFrameViewContentClip
> createOrUpdateFrameViewScrollTranslation
> createOrUpdateFrameViewScroll
> 
> (BTW, all of this will go away with RootLayerScrolls)

What I mean is to have a method like ensureScroll() so that you don't have to
check whether the scroll was set or not.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1866: " 
<div id='positionedOverflow'>"
On 2016/09/08 at 23:16:26, pdr. wrote:
> On 2016/09/08 at 20:43:59, chrishtr wrote:
> > Also add a fixed-pos child. And another test that has an overflow:scroll
element that has position:absolute
> 
> Adding the fixed-pos test revealed a bug where we were creating a scroll node
for all FrameViews but only need to create them if frameView.isScrollable is
true. Fixed the bug and added the test.
> 
> I think this test covers the overflow:scroll element with position absolute.

It only has a child with absolute position. I'm suggesting that the
oveflow:scroll element have it in another test, to verify an
absolutely-positioned child is scrolled.

[pdr., 2016-09-09 21:33:22]

On 2016/09/09 at 16:27:28, chrishtr wrote:
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
> On 2016/09/08 at 23:16:26, pdr. wrote:
> > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > Comments explaining the difference between scroll and root scroll?
> > 
> > The root nodes on FrameView just store no-op paint properties to match cc's
trees and to avoid using null for the root. This looks pretty silly for the
scroll tree.
> > 
> > I'd like to revisit the non-null root property tree node discussion...
> > 
> > Because we already differ from cc's trees at the root nodes (e.g., no
viewport transform yet), I think we should actually remove the no-op root nodes
after all and use null for the root of all trees. This would avoid needing to
add a scrollable=false bit just for the root scroll node. I could do this as a
patch before this patch. WDYT?
> > 
> > The primary downside to removing no-op root nodes is having to tell szager
he was right.
> 
> If you want to do that, I'd suggest after this patch rather than before.
Forward progess.

I've filed 645615 to do this and added a todo in the code to do this. I plan to
do it as an immediate followup.

> 
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:92: if
(ScrollPaintPropertyNode* existingScroll = frameView.scroll())
> On 2016/09/08 at 23:16:26, pdr. wrote:
> > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > Maybe add frameview.createOrUpdateScroll() to avoid this conditional?
> > 
> > I'd like to avoid putting property tree logic on Layout classes, sort of
like how put paint logic in separate painter classes. For LayoutObject we use
ObjectPaintProperties, but we've been managing FrameView's sloppily in this
function.
> > 
> > How does breaking these createOrUpdates out into helpers in
PaintPropertyTreeBuilder sound to you? In the latest patch I moved the logic
into 4 new functions:
> > createOrUpdateFrameViewPreTranslation
> > createOrUpdateFrameViewContentClip
> > createOrUpdateFrameViewScrollTranslation
> > createOrUpdateFrameViewScroll
> > 
> > (BTW, all of this will go away with RootLayerScrolls)
> 
> What I mean is to have a method like ensureScroll() so that you don't have to
check whether the scroll was set or not.

Because this will be going away in a followup, would you be okay with the
createOrUpdate* approach? This more closely follows the pattern used in
ObjectPaintProperties.

> 
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> 
>
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1866: " 
<div id='positionedOverflow'>"
> On 2016/09/08 at 23:16:26, pdr. wrote:
> > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > Also add a fixed-pos child. And another test that has an overflow:scroll
element that has position:absolute
> > 
> > Adding the fixed-pos test revealed a bug where we were creating a scroll
node for all FrameViews but only need to create them if frameView.isScrollable
is true. Fixed the bug and added the test.
> > 
> > I think this test covers the overflow:scroll element with position absolute.
> 
> It only has a child with absolute position. I'm suggesting that the
oveflow:scroll element have it in another test, to verify an
absolutely-positioned child is scrolled.

Cool, done in NestedPositionedScrollProperties

[chrishtr, 2016-09-09 21:36:21]

On 2016/09/09 at 21:33:22, pdr wrote:
> On 2016/09/09 at 16:27:28, chrishtr wrote:
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/FrameView.h (right):
> > 
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
> > On 2016/09/08 at 23:16:26, pdr. wrote:
> > > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > > Comments explaining the difference between scroll and root scroll?
> > > 
> > > The root nodes on FrameView just store no-op paint properties to match
cc's trees and to avoid using null for the root. This looks pretty silly for the
scroll tree.
> > > 
> > > I'd like to revisit the non-null root property tree node discussion...
> > > 
> > > Because we already differ from cc's trees at the root nodes (e.g., no
viewport transform yet), I think we should actually remove the no-op root nodes
after all and use null for the root of all trees. This would avoid needing to
add a scrollable=false bit just for the root scroll node. I could do this as a
patch before this patch. WDYT?
> > > 
> > > The primary downside to removing no-op root nodes is having to tell szager
he was right.
> > 
> > If you want to do that, I'd suggest after this patch rather than before.
Forward progess.
> 
> I've filed 645615 to do this and added a todo in the code to do this. I plan
to do it as an immediate followup.
> 
> > 
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> > 
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:92: if
(ScrollPaintPropertyNode* existingScroll = frameView.scroll())
> > On 2016/09/08 at 23:16:26, pdr. wrote:
> > > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > > Maybe add frameview.createOrUpdateScroll() to avoid this conditional?
> > > 
> > > I'd like to avoid putting property tree logic on Layout classes, sort of
like how put paint logic in separate painter classes. For LayoutObject we use
ObjectPaintProperties, but we've been managing FrameView's sloppily in this
function.
> > > 
> > > How does breaking these createOrUpdates out into helpers in
PaintPropertyTreeBuilder sound to you? In the latest patch I moved the logic
into 4 new functions:
> > > createOrUpdateFrameViewPreTranslation
> > > createOrUpdateFrameViewContentClip
> > > createOrUpdateFrameViewScrollTranslation
> > > createOrUpdateFrameViewScroll
> > > 
> > > (BTW, all of this will go away with RootLayerScrolls)
> > 
> > What I mean is to have a method like ensureScroll() so that you don't have
to check whether the scroll was set or not.
> 
> Because this will be going away in a followup, would you be okay with the
createOrUpdate* approach? This more closely follows the pattern used in
ObjectPaintProperties.


Ok.
> 
> > 
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> > 
> >
https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:1866:
"  <div id='positionedOverflow'>"
> > On 2016/09/08 at 23:16:26, pdr. wrote:
> > > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > > Also add a fixed-pos child. And another test that has an overflow:scroll
element that has position:absolute
> > > 
> > > Adding the fixed-pos test revealed a bug where we were creating a scroll
node for all FrameViews but only need to create them if frameView.isScrollable
is true. Fixed the bug and added the test.
> > > 
> > > I think this test covers the overflow:scroll element with position
absolute.
> > 
> > It only has a child with absolute position. I'm suggesting that the
oveflow:scroll element have it in another test, to verify an
absolutely-positioned child is scrolled.
> 
> Cool, done in NestedPositionedScrollProperties

[chrishtr, 2016-09-09 21:36:25]

lgtm

[pdr., 2016-09-09 21:41:36]

On 2016/09/09 at 21:36:25, chrishtr wrote:
> lgtm

Thanks all! In we go.

[pdr., 2016-09-09 21:41:38]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-09-09 21:42:22]

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

[trchen, 2016-09-09 22:03:17]

BTW non-owner LGTM too.

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2320463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:621: void
setRootScroll(PassRefPtr<ScrollPaintPropertyNode> rootScroll) { m_rootScroll =
rootScroll; }
On 2016/09/09 16:27:28, chrishtr wrote:
> On 2016/09/08 at 23:16:26, pdr. wrote:
> > On 2016/09/08 at 20:43:59, chrishtr wrote:
> > > Comments explaining the difference between scroll and root scroll?
> > 
> > The root nodes on FrameView just store no-op paint properties to match cc's
> trees and to avoid using null for the root. This looks pretty silly for the
> scroll tree.
> > 
> > I'd like to revisit the non-null root property tree node discussion...
> > 
> > Because we already differ from cc's trees at the root nodes (e.g., no
viewport
> transform yet), I think we should actually remove the no-op root nodes after
all
> and use null for the root of all trees. This would avoid needing to add a
> scrollable=false bit just for the root scroll node. I could do this as a patch
> before this patch. WDYT?

+1 The decision needs to be revisited.

IMO the decision to add no-op root nodes at blink property tree builder was not
very well thought (and I contributed part of the mess. Sorry.).

In my mind model the nullptr blink property node should just mean "whatever the
inherited context was". For example, for a top level compositor, nullptr
transform node means the window space, while for a OoP-frame it refers to the
transform space of the embedding foreign display item.

Also from the perspective of cc, property nodes #0 basically don't carry any
operation themselves, but just provide the context to be used for drawing or
getting inherited by child nodes. For example:
this.screen_space_transform = this.local_transform *
parent.screen_space_transform
In this case "local_transform" is an operation to modify
"screen_space_transform". And transform node #0's screen_space_transform is
hard-coded to be the device scale factor.

I think a cleaner model is that nullptr should just map to cc property node #0.
There are some cases that property node #1 has hard-coded operations. IMO we
should eliminate those special cases in cc. In the meantime we can generate
dummy nodes in PAC, and let nullptr maps to the dummy node.

> > The primary downside to removing no-op root nodes is having to tell szager
he
> was right.
> 
> If you want to do that, I'd suggest after this patch rather than before.
Forward
> progess.

+1 we can do it after.

[pdr., 2016-09-09 22:08:38]

On 2016/09/09 at 22:03:17, trchen wrote:
> BTW non-owner LGTM too.

Non-owner? O_o

git log --author="trchen" --pretty=oneline | wc -l
174

Your reviews are good, we should fix that non-owner thing.

[commit-bot: I haz the power, 2016-09-10 00:06:43]

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

[commit-bot: I haz the power, 2016-09-10 00:06:44]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2016-09-10 00:19:29]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-09-10 00:20:00]

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

[pdr., 2016-09-10 02:27:46]

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

[commit-bot: I haz the power, 2016-09-10 02:28:01]

Dry run: 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, 2016-09-10 03:26:43]

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

[commit-bot: I haz the power, 2016-09-10 03:26:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2016-09-10 05:46:39]

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

[commit-bot: I haz the power, 2016-09-10 05:46:43]

Dry run: 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, 2016-09-10 08:49:10]

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

[commit-bot: I haz the power, 2016-09-10 08:49:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pdr., 2016-09-10 18:10:54]

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

[commit-bot: I haz the power, 2016-09-10 18:10:59]

Dry run: 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, 2016-09-10 18:46:47]

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

[commit-bot: I haz the power, 2016-09-10 18:46:48]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-11 00:09:11]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-09-11 00:09:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2320463002/#ps100001
(title: "Prevent circular reference caught by lsan")

[commit-bot: I haz the power, 2016-09-11 00:09:19]

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, 2016-09-11 00:13:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-11 00:16:36]

Description was changed from

==========
[SPV2] Implement the blink-side scroll property tree

Blink's scroll tree stores auxiliary scrolling information for
compositor-thread scrolling. This includes the hierarchy of scrolling
(which areas scroll relative to which other areas), whether an area
is user-scrollable in the x or y axes, and the extent that an area
can be scrolled. This patch is an initial implementation of the
blink scroll paint property tree with documentation, a followup will
plumb this to the compositor.

A proof-of-concept for the end-to-end design is available at:
https://codereview.chromium.org/2299533002

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

to

==========
[SPV2] Implement the blink-side scroll property tree

Blink's scroll tree stores auxiliary scrolling information for
compositor-thread scrolling. This includes the hierarchy of scrolling
(which areas scroll relative to which other areas), whether an area
is user-scrollable in the x or y axes, and the extent that an area
can be scrolled. This patch is an initial implementation of the
blink scroll paint property tree with documentation, a followup will
plumb this to the compositor.

A proof-of-concept for the end-to-end design is available at:
https://codereview.chromium.org/2299533002

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

Committed: https://crrev.com/d1a5fe1b572bf7b446d45558823ac7a8d759628c
Cr-Commit-Position: refs/heads/master@{#417850}
==========

[commit-bot: I haz the power, 2016-09-11 00:16:38]

Patchset 6 (id:??) landed as
https://crrev.com/d1a5fe1b572bf7b446d45558823ac7a8d759628c
Cr-Commit-Position: refs/heads/master@{#417850}