Add position sticky main thread scrolling reason [spv2]

Add position sticky main thread scrolling reason [spv2]

This patch builds on [1] and adds position: sticky main thread scrolling
reasons to the blink and cc scroll property trees. Position: sticky
disables threaded scrolling because the compositor is not yet
able to update these.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

[1] https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
BUG=644514
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[pdr., 2016-09-20 01:34:08]

Description was changed from

==========
Add position sticky main thread scrolling reason [spv2]

This patch builds on [1] and adds position: sticky main thread scrolling
reasons to the blink and cc scroll property trees. Position: sticky
disables threaded scrolling because the compositor is not yet
able to update these.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

[1] https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
BUG=644514
==========

to

==========
Add position sticky main thread scrolling reason [spv2]

This patch builds on [1] and adds position: sticky main thread scrolling
reasons to the blink and cc scroll property trees. Position: sticky
disables threaded scrolling because the compositor is not yet
able to update these.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

[1] https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
BUG=644514
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-09-20 01:34:25]

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

[commit-bot: I haz the power, 2016-09-20 01:35:02]

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-20 03:06:54]

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

[commit-bot: I haz the power, 2016-09-20 03:06:55]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-20 16:45:56]

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

[chrishtr, 2016-09-20 19:45:57]

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:375:
ancestorOverflowLayer->getScrollableArea()->invalidateStickyConstraintsFor(layer());
There is a call site in PaintLayer also.

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

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2135: " 
<div id='overflowB' class='positionSticky'>"
It's a little weird that you put position:sticky on the scroll itself. Child
instead?

[pdr., 2016-09-20 20:16:58]

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:375:
ancestorOverflowLayer->getScrollableArea()->invalidateStickyConstraintsFor(layer());
On 2016/09/20 at 19:45:57, chrishtr wrote:
> There is a call site in PaintLayer also.

Added a comment there.

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

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2135: " 
<div id='overflowB' class='positionSticky'>"
On 2016/09/20 at 19:45:57, chrishtr wrote:
> It's a little weird that you put position:sticky on the scroll itself. Child
instead?

It is weird and I doubt anyone will do it, but I think it's important to test
the case where the scroller itself is sticky. This same issue took me a while to
work through and explain for background attachment fixed
(https://codereview.chromium.org/2345403003#msg6), and this test is the same
except for sticky. The tldr version is: when we declare something as sticky,
it's scrolled contents are not necessarily sticky but it's parent scroll node is
sticky. This is why we expect_false that overflow b's scroll node has sticky
positioned objects (line 2146), even though overflow b itself is sticky.

Here's real example of the situation: http://output.jsbin.com/foheli
(requires --enable-experimental-web-platform-features since sticky has not
shipped)

[chrishtr, 2016-09-20 20:29:35]

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

https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2135: " 
<div id='overflowB' class='positionSticky'>"
On 2016/09/20 at 20:16:57, pdr. wrote:
> On 2016/09/20 at 19:45:57, chrishtr wrote:
> > It's a little weird that you put position:sticky on the scroll itself. Child
instead?
> 
> It is weird and I doubt anyone will do it, but I think it's important to test
the case where the scroller itself is sticky. This same issue took me a while to
work through and explain for background attachment fixed
(https://codereview.chromium.org/2345403003#msg6), and this test is the same
except for sticky. The tldr version is: when we declare something as sticky,
it's scrolled contents are not necessarily sticky but it's parent scroll node is
sticky. This is why we expect_false that overflow b's scroll node has sticky
positioned objects (line 2146), even though overflow b itself is sticky.
> 
> Here's real example of the situation: http://output.jsbin.com/foheli
> (requires --enable-experimental-web-platform-features since sticky has not
shipped)

Ok makes sense. Please also add a test for an element within the scroller.

[pdr., 2016-09-20 20:45:42]

On 2016/09/20 at 20:29:35, chrishtr wrote:
>
https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> 
>
https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2135: " 
<div id='overflowB' class='positionSticky'>"
> On 2016/09/20 at 20:16:57, pdr. wrote:
> > On 2016/09/20 at 19:45:57, chrishtr wrote:
> > > It's a little weird that you put position:sticky on the scroll itself.
Child instead?
> > 
> > It is weird and I doubt anyone will do it, but I think it's important to
test the case where the scroller itself is sticky. This same issue took me a
while to work through and explain for background attachment fixed
(https://codereview.chromium.org/2345403003#msg6), and this test is the same
except for sticky. The tldr version is: when we declare something as sticky,
it's scrolled contents are not necessarily sticky but it's parent scroll node is
sticky. This is why we expect_false that overflow b's scroll node has sticky
positioned objects (line 2146), even though overflow b itself is sticky.
> > 
> > Here's real example of the situation: http://output.jsbin.com/foheli
> > (requires --enable-experimental-web-platform-features since sticky has not
shipped)
> 
> Ok makes sense. Please also add a test for an element within the scroller.

Good idea, done

[pdr., 2016-09-20 21:49:01]

On 2016/09/20 at 20:45:42, pdr. wrote:
> On 2016/09/20 at 20:29:35, chrishtr wrote:
> >
https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> > 
> >
https://codereview.chromium.org/2353843002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2135:
"  <div id='overflowB' class='positionSticky'>"
> > On 2016/09/20 at 20:16:57, pdr. wrote:
> > > On 2016/09/20 at 19:45:57, chrishtr wrote:
> > > > It's a little weird that you put position:sticky on the scroll itself.
Child instead?
> > > 
> > > It is weird and I doubt anyone will do it, but I think it's important to
test the case where the scroller itself is sticky. This same issue took me a
while to work through and explain for background attachment fixed
(https://codereview.chromium.org/2345403003#msg6), and this test is the same
except for sticky. The tldr version is: when we declare something as sticky,
it's scrolled contents are not necessarily sticky but it's parent scroll node is
sticky. This is why we expect_false that overflow b's scroll node has sticky
positioned objects (line 2146), even though overflow b itself is sticky.
> > > 
> > > Here's real example of the situation: http://output.jsbin.com/foheli
> > > (requires --enable-experimental-web-platform-features since sticky has not
shipped)
> > 
> > Ok makes sense. Please also add a test for an element within the scroller.
> 
> Good idea, done

I misunderstood what you wanted here and added a test of a non-sticky child of a
sticky scroller. I've left that test in because it is still useful.

In the latest patch I've modified
PositionStickyMainThreadScrollReasonsWithNestedScrollers to have a non-scrolling
sticky child, and ensured that the kHasStickyPositionObjects bit is propagated
up.

[pdr., 2016-09-20 22:33:36]

PTAL

[chrishtr, 2016-09-20 22:43:46]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-09-20 22:43:47]

lgtm

[commit-bot: I haz the power, 2016-09-20 22:44:47]

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-20 22:48:17]

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

[commit-bot: I haz the power, 2016-09-20 22:48:18]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pdr., 2016-09-20 23:24:27]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-09-20 23:24:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2353843002/#ps80001
(title: "Embrace the rebase")

[commit-bot: I haz the power, 2016-09-20 23:25:05]

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-21 02:06:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-21 02:09:37]

Description was changed from

==========
Add position sticky main thread scrolling reason [spv2]

This patch builds on [1] and adds position: sticky main thread scrolling
reasons to the blink and cc scroll property trees. Position: sticky
disables threaded scrolling because the compositor is not yet
able to update these.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

[1] https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
BUG=644514
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add position sticky main thread scrolling reason [spv2]

This patch builds on [1] and adds position: sticky main thread scrolling
reasons to the blink and cc scroll property trees. Position: sticky
disables threaded scrolling because the compositor is not yet
able to update these.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

[1] https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
BUG=644514
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-09-21 02:09:39]

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