Composite sticky-positioned elements when they have composited descendants

Composite sticky-positioned elements when they have composited descendants

In the case of nested sticky elements, if the descendant elements are promoted
but their ancestor is not we will position them incorrectly. Instead of trying
to re-compute main-thread positioning on the compositor, we can just promote
any sticky element that has promoted sticky descendants.

This CL goes slightly further and promotes any sticky element that has
composited descendants regardless of whether they are sticky - doing so makes
the code much simpler and the performance impact should be limited by the fact
that sticky is not common.

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

Review-Url: https://codereview.chromium.org/2754983002
Cr-Commit-Position: refs/heads/master@{#471217}
Committed: https://chromium.googlesource.com/chromium/src/+/de4c11424c87b46a8f5ce0935f9367a868baa507

[smcgruer, 2017-03-16 20:24:01]

Description was changed from

==========
Composite sticky-positioned elements when they have composited descendants

In the case of nested sticky elements, if the descendant elements are promoted
but their ancestor is not we will position them incorrectly. Instead of trying
to re-compute main-thread positioning on the compositor, we can just promote
any sticky element that has promoted sticky descendants.

This CL goes slightly further and promotes any sticky element that has
composited descendants regardless of whether they are sticky - doing so makes
the code much simpler and the performance impact should be limited by the fact
that sticky is not common.

BUG=702229
==========

to

==========
Composite sticky-positioned elements when they have composited descendants

In the case of nested sticky elements, if the descendant elements are promoted
but their ancestor is not we will position them incorrectly. Instead of trying
to re-compute main-thread positioning on the compositor, we can just promote
any sticky element that has promoted sticky descendants.

This CL goes slightly further and promotes any sticky element that has
composited descendants regardless of whether they are sticky - doing so makes
the code much simpler and the performance impact should be limited by the fact
that sticky is not common.

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

[smcgruer, 2017-03-21 15:02:32]

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

[commit-bot: I haz the power, 2017-03-21 15:02:46]

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

[smcgruer, 2017-03-21 15:02:55]

smcgruer@chromium.org changed reviewers:
+ flackr@chromium.org

[smcgruer, 2017-03-21 15:03:22]

This likely depends-on your patch to be submitted first, but throwing it out
there as an FYI anyway :)

[commit-bot: I haz the power, 2017-03-21 16:52:03]

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

[commit-bot: I haz the power, 2017-03-21 16:52:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[flackr, 2017-03-23 17:30:58]

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

https://codereview.chromium.org/2754983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:197:
CompositingReasonPositionFixedOrStickyWithCompositedDescendants;
Will this cause the scroller to be composited too? If not we need to test
composited sticky in non-composited overflow scrollers.

[smcgruer, 2017-03-24 18:28:28]

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

https://codereview.chromium.org/2754983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:197:
CompositingReasonPositionFixedOrStickyWithCompositedDescendants;
On 2017/03/23 17:30:58, flackr wrote:
> Will this cause the scroller to be composited too? If not we need to test
> composited sticky in non-composited overflow scrollers.

It will not (based on testing http://output.jsbin.com/kahixo), but that
situation should work. Added layout test for it - do you think it needs a
unittest too?

[flackr, 2017-04-03 17:27:59]

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

https://codereview.chromium.org/2754983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:197:
CompositingReasonPositionFixedOrStickyWithCompositedDescendants;
On 2017/03/24 18:28:28, smcgruer wrote:
> On 2017/03/23 17:30:58, flackr wrote:
> > Will this cause the scroller to be composited too? If not we need to test
> > composited sticky in non-composited overflow scrollers.
> 
> It will not (based on testing http://output.jsbin.com/kahixo), but that
> situation should work. Added layout test for it - do you think it needs a
> unittest too?

No I think just having the layout test is fine.

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:193:
// TODO(smcgruer): Use hasStickyConstrainedPosition() from flackr's patch
I actually feel like we should call
CompositingReasonFinder::requiresCompositingForScrollDependentPosition instead,
since this is trying to duplicate the checks for sticky / fixed promotion there.

[smcgruer, 2017-04-03 19:59:53]

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:193:
// TODO(smcgruer): Use hasStickyConstrainedPosition() from flackr's patch
On 2017/04/03 17:27:59, flackr wrote:
> I actually feel like we should call
> CompositingReasonFinder::requiresCompositingForScrollDependentPosition
instead,
> since this is trying to duplicate the checks for sticky / fixed promotion
there.

That would slightly the existing fixed behavior here though. It would add the
opaque-fixed-pos check and the 'layer->fixedToViewport() &&
m_layoutView.frameView()->isScrollable()' check. Are we sure that is desirable?
(I have no idea at first glance :).)

[flackr, 2017-04-07 18:50:43]

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:193:
// TODO(smcgruer): Use hasStickyConstrainedPosition() from flackr's patch
On 2017/04/03 19:59:53, smcgruer wrote:
> On 2017/04/03 17:27:59, flackr wrote:
> > I actually feel like we should call
> > CompositingReasonFinder::requiresCompositingForScrollDependentPosition
> instead,
> > since this is trying to duplicate the checks for sticky / fixed promotion
> there.
> 
> That would slightly the existing fixed behavior here though. It would add the
> opaque-fixed-pos check and the 'layer->fixedToViewport() &&
> m_layoutView.frameView()->isScrollable()' check. Are we sure that is
desirable?
> (I have no idea at first glance :).)

fixedToViewport doesn't check for opaqueness, just whether the fixed position
element is contained (scrolls) or not (fixed). Since it doesn't move with
respect to the scrolling contents if contained it makes sense not to promote it
as we're doing with sticky.

[flackr, 2017-04-09 04:07:02]

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:193:
// TODO(smcgruer): Use hasStickyConstrainedPosition() from flackr's patch
On 2017/04/07 18:50:43, flackr wrote:
> On 2017/04/03 19:59:53, smcgruer wrote:
> > On 2017/04/03 17:27:59, flackr wrote:
> > > I actually feel like we should call
> > > CompositingReasonFinder::requiresCompositingForScrollDependentPosition
> > instead,
> > > since this is trying to duplicate the checks for sticky / fixed promotion
> > there.
> > 
> > That would slightly the existing fixed behavior here though. It would add
the
> > opaque-fixed-pos check and the 'layer->fixedToViewport() &&
> > m_layoutView.frameView()->isScrollable()' check. Are we sure that is
> desirable?
> > (I have no idea at first glance :).)
> 
> fixedToViewport doesn't check for opaqueness, just whether the fixed position
> element is contained (scrolls) or not (fixed). Since it doesn't move with
> respect to the scrolling contents if contained it makes sense not to promote
it
> as we're doing with sticky.

Sorry, meant to say we call it with ignoreLCDText = true which won't consider
opaqueness.

[smcgruer, 2017-04-21 15:17:32]

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:193:
// TODO(smcgruer): Use hasStickyConstrainedPosition() from flackr's patch
On 2017/04/09 04:07:02, flackr wrote:
> On 2017/04/07 18:50:43, flackr wrote:
> > On 2017/04/03 19:59:53, smcgruer wrote:
> > > On 2017/04/03 17:27:59, flackr wrote:
> > > > I actually feel like we should call
> > > > CompositingReasonFinder::requiresCompositingForScrollDependentPosition
> > > instead,
> > > > since this is trying to duplicate the checks for sticky / fixed
promotion
> > > there.
> > > 
> > > That would slightly the existing fixed behavior here though. It would add
> the
> > > opaque-fixed-pos check and the 'layer->fixedToViewport() &&
> > > m_layoutView.frameView()->isScrollable()' check. Are we sure that is
> > desirable?
> > > (I have no idea at first glance :).)
> > 
> > fixedToViewport doesn't check for opaqueness, just whether the fixed
position
> > element is contained (scrolls) or not (fixed). Since it doesn't move with
> > respect to the scrolling contents if contained it makes sense not to promote
> it
> > as we're doing with sticky.
> 
> Sorry, meant to say we call it with ignoreLCDText = true which won't consider
> opaqueness.

Done.

[smcgruer, 2017-04-21 15:18:08]

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

[commit-bot: I haz the power, 2017-04-21 15:18:24]

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, 2017-04-21 16:15:30]

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

[commit-bot: I haz the power, 2017-04-21 16:15:31]

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

[flackr, 2017-04-26 15:42:36]

LGTM

https://codereview.chromium.org/2754983002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:195:
layer, true)) {
nit: the style, depending on where blink is at now, for annotating bools in
method calls is either:
RequiresCompositingForScrollDependentPosition(layer, true /* ignore_lcd_text */)
or
const bool ignore_lcd_text = true;
... RequiresCompositingForScrollDependentPosition(layer, ignore_lcd_text).

[smcgruer, 2017-04-26 18:05:46]

Unfortunately this CL does cause a true failure in
compositing/always-composite-fixed-position-when-descendants-composite.html -
the fixed element is no longer composited.

Investigating!

https://codereview.chromium.org/2754983002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2754983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:195:
layer, true)) {
On 2017/04/26 15:42:36, flackr wrote:
> nit: the style, depending on where blink is at now, for annotating bools in
> method calls is either:
> RequiresCompositingForScrollDependentPosition(layer, true /* ignore_lcd_text
*/)
> or
> const bool ignore_lcd_text = true;
> ... RequiresCompositingForScrollDependentPosition(layer, ignore_lcd_text).

Done.

[smcgruer, 2017-04-26 18:15:11]

On 2017/04/26 18:05:46, smcgruer wrote:
> Unfortunately this CL does cause a true failure in
> compositing/always-composite-fixed-position-when-descendants-composite.html -
> the fixed element is no longer composited.
> 
> Investigating!

So the issue at a high level is that
RequiresCompositingForScrollDependentPosition does the following for fixed:

if (position == EPosition::kFixed)
    return layer->FixedToViewport() &&
layout_view_.GetFrameView()->IsScrollable();

These are both new checks with this CL (previously the logic here was just fixed
==> 'true'), and in the particular case of this CL, the latter isn't true.
Nothing is scrollable, so we don't composite it.

Any thoughts on whether the test is wrong or this CL is wrong?

[flackr, 2017-04-26 18:39:08]

On 2017/04/26 18:15:11, smcgruer wrote:
> On 2017/04/26 18:05:46, smcgruer wrote:
> > Unfortunately this CL does cause a true failure in
> > compositing/always-composite-fixed-position-when-descendants-composite.html
-
> > the fixed element is no longer composited.
> > 
> > Investigating!
> 
> So the issue at a high level is that
> RequiresCompositingForScrollDependentPosition does the following for fixed:
> 
> if (position == EPosition::kFixed)
>     return layer->FixedToViewport() &&
> layout_view_.GetFrameView()->IsScrollable();
> 
> These are both new checks with this CL (previously the logic here was just
fixed
> ==> 'true'), and in the particular case of this CL, the latter isn't true.
> Nothing is scrollable, so we don't composite it.
> 
> Any thoughts on whether the test is wrong or this CL is wrong?

The old behavior seems incorrect, there should be no reason to promote a fixed
position ancestor that we didn't already promote if it's not actually going to
move with the scroll. We could separate this change from adding sticky to the
ancestor promotion rules though.

[flackr, 2017-04-26 18:39:52]

On 2017/04/26 18:39:08, flackr wrote:
> On 2017/04/26 18:15:11, smcgruer wrote:
> > On 2017/04/26 18:05:46, smcgruer wrote:
> > > Unfortunately this CL does cause a true failure in
> > >
compositing/always-composite-fixed-position-when-descendants-composite.html
> -
> > > the fixed element is no longer composited.
> > > 
> > > Investigating!
> > 
> > So the issue at a high level is that
> > RequiresCompositingForScrollDependentPosition does the following for fixed:
> > 
> > if (position == EPosition::kFixed)
> >     return layer->FixedToViewport() &&
> > layout_view_.GetFrameView()->IsScrollable();
> > 
> > These are both new checks with this CL (previously the logic here was just
> fixed
> > ==> 'true'), and in the particular case of this CL, the latter isn't true.
> > Nothing is scrollable, so we don't composite it.
> > 
> > Any thoughts on whether the test is wrong or this CL is wrong?
> 
> The old behavior seems incorrect, there should be no reason to promote a fixed
> position ancestor that we didn't already promote if it's not actually going to
> move with the scroll. We could separate this change from adding sticky to the
> ancestor promotion rules though.

Alternately, we can modify the tests so that the fixed pos elements do move with
scroll.

[smcgruer, 2017-04-26 18:42:10]

On 2017/04/26 18:39:52, flackr wrote:
> On 2017/04/26 18:39:08, flackr wrote:
> > The old behavior seems incorrect, there should be no reason to promote a
fixed
> > position ancestor that we didn't already promote if it's not actually going
to
> > move with the scroll. We could separate this change from adding sticky to
the
> > ancestor promotion rules though.
> 
> Alternately, we can modify the tests so that the fixed pos elements do move
with
> scroll.

I'll split the CL, open a new bug for position: fixed and fix that first.
Keeping it together muddies the purpose of this CL, which is to fix a specific
position: sticky bug.

[smcgruer, 2017-05-05 14:14:30]

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

[commit-bot: I haz the power, 2017-05-05 14:14:46]

Dry run: CQ is trying da patch.

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

[smcgruer, 2017-05-05 14:23:09]

On 2017/04/26 18:42:10, smcgruer wrote:
> On 2017/04/26 18:39:52, flackr wrote:
> > On 2017/04/26 18:39:08, flackr wrote:
> > > The old behavior seems incorrect, there should be no reason to promote a
> fixed
> > > position ancestor that we didn't already promote if it's not actually
going
> to
> > > move with the scroll. We could separate this change from adding sticky to
> the
> > > ancestor promotion rules though.
> > 
> > Alternately, we can modify the tests so that the fixed pos elements do move
> with
> > scroll.
> 
> I'll split the CL, open a new bug for position: fixed and fix that first.
> Keeping it together muddies the purpose of this CL, which is to fix a specific
> position: sticky bug.

Done, although yigu@ mentioned on http://crbug.com/702229 that it's possible to
have an out-of-order traversal here? If so, we should try and construct an
example where this CL would fail.

[commit-bot: I haz the power, 2017-05-05 16:04:30]

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

[commit-bot: I haz the power, 2017-05-05 16:04:31]

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

[smcgruer, 2017-05-05 16:27:43]

This is amusing. The buildbots fail because SPv2 incorrectly renders the
-expected.html image... I assume I should just file a SPv2 bug and add the test
to the SPv2 skip list?

[smcgruer, 2017-05-05 17:19:44]

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

[commit-bot: I haz the power, 2017-05-05 17:20:34]

Dry run: CQ is trying da patch.

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

[pdr., 2017-05-05 17:28:30]

On 2017/05/05 at 16:27:43, smcgruer wrote:
> This is amusing. The buildbots fail because SPv2 incorrectly renders the
-expected.html image... I assume I should just file a SPv2 bug and add the test
to the SPv2 skip list?

Yeah, just mark the test as failing. No need to write "# incorrectly renders the
-expected.html", even.

[smcgruer, 2017-05-05 17:49:05]

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

[commit-bot: I haz the power, 2017-05-05 17:49:46]

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, 2017-05-05 19:01:45]

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

[commit-bot: I haz the power, 2017-05-05 19:01:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[smcgruer, 2017-05-05 19:03:06]

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

[commit-bot: I haz the power, 2017-05-05 19:03:52]

Dry run: CQ is trying da patch.

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

[smcgruer, 2017-05-05 21:20:05]

smcgruer@chromium.org changed reviewers:
+ chrishtr@chromium.org

[smcgruer, 2017-05-05 21:20:06]

+Chris for Blink OWNERS

[commit-bot: I haz the power, 2017-05-05 23:50:33]

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

[commit-bot: I haz the power, 2017-05-05 23:50:34]

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

[smcgruer, 2017-05-06 13:43:47]

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

[commit-bot: I haz the power, 2017-05-06 13:44:15]

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, 2017-05-06 14:19:18]

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

[commit-bot: I haz the power, 2017-05-06 14:19:19]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-05-11 14:08:11]

Ping for Blink OWNERS

[chrishtr, 2017-05-11 14:54:46]

Sorry fro the delay.

https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:322:
Bug(none)
compositing/overflow/mixed-composited-nested-sticky-overflow-scroller.html [
Failure ]
Why is this failing?

[smcgruer, 2017-05-11 15:05:03]

https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:322:
Bug(none)
compositing/overflow/mixed-composited-nested-sticky-overflow-scroller.html [
Failure ]
On 2017/05/11 14:54:46, chrishtr wrote:
> Why is this failing?

As per the comment that pdr@ asked me to remove ;), SPv2 can't render the
expected html correctly. See http://crbug.com/718971

[chrishtr, 2017-05-11 15:05:35]

On 2017/05/11 at 15:05:03, smcgruer wrote:
>
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):
> 
>
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:322:
Bug(none)
compositing/overflow/mixed-composited-nested-sticky-overflow-scroller.html [
Failure ]
> On 2017/05/11 14:54:46, chrishtr wrote:
> > Why is this failing?
> 
> As per the comment that pdr@ asked me to remove ;), SPv2 can't render the
expected html correctly. See http://crbug.com/718971

Oh SPv2, right. Didn't look closely enough.

[chrishtr, 2017-05-11 15:05:39]

lgtm

[smcgruer, 2017-05-11 15:06:07]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-05-11 15:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2754983002/#ps140001
(title: "Address pdr comment")

[chrishtr, 2017-05-11 15:06:15]

On 2017/05/11 at 15:05:35, chrishtr wrote:
> On 2017/05/11 at 15:05:03, smcgruer wrote:
> >
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):
> > 
> >
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:322:
Bug(none)
compositing/overflow/mixed-composited-nested-sticky-overflow-scroller.html [
Failure ]
> > On 2017/05/11 14:54:46, chrishtr wrote:
> > > Why is this failing?
> > 
> > As per the comment that pdr@ asked me to remove ;), SPv2 can't render the
expected html correctly. See http://crbug.com/718971
> 
> Oh SPv2, right. Didn't look closely enough.

Actually, could you mark that bug in enable-slimming-paint-v2 instead of
Bug(none)?

[commit-bot: I haz the power, 2017-05-11 15:06:34]

CQ is trying da patch.

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

[smcgruer, 2017-05-11 15:10:19]

The CQ bit was unchecked by smcgruer@chromium.org

[smcgruer, 2017-05-11 15:21:02]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-05-11 15:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2754983002/#ps160001
(title: "Address chrishtr comment")

[smcgruer, 2017-05-11 15:21:17]

On 2017/05/11 15:06:15, chrishtr wrote:
> On 2017/05/11 at 15:05:35, chrishtr wrote:
> > On 2017/05/11 at 15:05:03, smcgruer wrote:
> > >
>
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> > > File
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2754983002/diff/140001/third_party/WebKit/Lay...
> > >
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:322:
> Bug(none)
> compositing/overflow/mixed-composited-nested-sticky-overflow-scroller.html [
> Failure ]
> > > On 2017/05/11 14:54:46, chrishtr wrote:
> > > > Why is this failing?
> > > 
> > > As per the comment that pdr@ asked me to remove ;), SPv2 can't render the
> expected html correctly. See http://crbug.com/718971
> > 
> > Oh SPv2, right. Didn't look closely enough.
> 
> Actually, could you mark that bug in enable-slimming-paint-v2 instead of
> Bug(none)?

Done.

[commit-bot: I haz the power, 2017-05-11 15:21:28]

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-05-11 16:16:46]

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

[commit-bot: I haz the power, 2017-05-11 16:16:47]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[smcgruer, 2017-05-11 18:10:04]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:10:52]

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-05-11 20:01:56]

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

[commit-bot: I haz the power, 2017-05-11 20:01:57]

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

[smcgruer, 2017-05-11 20:15:59]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-05-11 20:17:09]

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-05-11 23:34:05]

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

[commit-bot: I haz the power, 2017-05-11 23:34:07]

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

[smcgruer, 2017-05-12 03:33:33]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-05-12 03:34:08]

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-05-12 04:17:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494560013472210,
"parent_rev": "d25e37df0f08a754fbc11041451a5ce7ca7d043d", "commit_rev":
"de4c11424c87b46a8f5ce0935f9367a868baa507"}

[commit-bot: I haz the power, 2017-05-12 04:17:14]

Description was changed from

==========
Composite sticky-positioned elements when they have composited descendants

In the case of nested sticky elements, if the descendant elements are promoted
but their ancestor is not we will position them incorrectly. Instead of trying
to re-compute main-thread positioning on the compositor, we can just promote
any sticky element that has promoted sticky descendants.

This CL goes slightly further and promotes any sticky element that has
composited descendants regardless of whether they are sticky - doing so makes
the code much simpler and the performance impact should be limited by the fact
that sticky is not common.

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

to

==========
Composite sticky-positioned elements when they have composited descendants

In the case of nested sticky elements, if the descendant elements are promoted
but their ancestor is not we will position them incorrectly. Instead of trying
to re-compute main-thread positioning on the compositor, we can just promote
any sticky element that has promoted sticky descendants.

This CL goes slightly further and promotes any sticky element that has
composited descendants regardless of whether they are sticky - doing so makes
the code much simpler and the performance impact should be limited by the fact
that sticky is not common.

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

Review-Url: https://codereview.chromium.org/2754983002
Cr-Commit-Position: refs/heads/master@{#471217}
Committed:
https://chromium.googlesource.com/chromium/src/+/de4c11424c87b46a8f5ce0935f93...
==========

[commit-bot: I haz the power, 2017-05-12 04:17:16]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/de4c11424c87b46a8f5ce0935f93...