Make composited scrolling codepaths co-operate.

Make composited scrolling codepaths co-operate.

Since we don’t have a functional layer-squashing solution
yet, it makes sense to leave in the logic for the stacking
container concept for the short term, and use it whenever
possible. If a layer cannot become a stacking container,
then we fall back to using the new codepath that’s being set
up in all the above patches.

This is blink-side patch 6 of https://docs.google.com/a/chromium.org/document/d/1Ln57_v6_4RmuuvtWnpsfBOUeDFrPsYNFW13dwuQ8UD8.

BUG=254111
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159168

[hartmanng, 2013-07-25 20:22:40]

[- blink-reviews@chromium.org, dglazkov+blink@chromium.org,
eae+blinkwatch@chromium.org, leviw+renderwatch@chromium.org,
jchaffraix+rendering@chromium.org to avoid spam]

all the layout test failures are fixed (except
compositing/overflow/do-not-opt-non-stacking-context-descendants-into-composited-scrolling.html
and
compositing/overflow/opt-unclipped-descendants-into-composited-scrolling.html -
I believe they rely on cc-side work to put unclipped descendants in the right
spot in the tree).

[hartmanng, 2013-07-26 15:32:13]

Last two layout tests are fixed (problem was that I was trying to treat
unclipped descendants the same as everything else, assuming that
RenderLayer::ancestorScrollingLayer() was relevant for them, which I don't think
it is). Should be a bit cleaner too (eg, spacing in layout tests, bool vs
unsigned, etc).

Also you were right - I thought I had made compositorDrivenAcceleratedScrolling
conditional on acceleratedCompositingForOverflowScroll, but I hadn't yet, that
should be fixed too.

Finally, I uncommented the check for isZOrdered, which I had also intended to do
already, but it looked from your patch like we may not need it in the new
version of scrollingAncestor() anyway?

[Ian Vollick, 2013-08-08 03:30:10]

I like it. Just one small question.

https://codereview.chromium.org/20103002/diff/34001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/34001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:509: if
(scrollingLayer->m_canBePromotedToStackingContainerDirty) {
Can you describe how we get into the situation where we're asking if we're using
compositor driven accelerated scrolling when can-be-promoted.. is dirty?

[hartmanng, 2013-08-08 13:41:37]

https://codereview.chromium.org/20103002/diff/34001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/34001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:509: if
(scrollingLayer->m_canBePromotedToStackingContainerDirty) {
On 2013/08/08 03:30:10, vollick wrote:
> Can you describe how we get into the situation where we're asking if we're
using
> compositor driven accelerated scrolling when can-be-promoted.. is dirty?

The situation I ran into looks like this:

 [0x7f795c414a78]
WebCore::RenderLayer::useCompositorDrivenAcceleratedScrolling()
 [0x7f795c452179]
WebCore::RenderLayerCompositor::requiresCompositingForOverflowScrollingParent()
 [0x7f795c44db86] WebCore::RenderLayerCompositor::directReasonsForCompositing()
 [0x7f795c44e8dd] WebCore::RenderLayerCompositor::needsToBeComposited()
 [0x7f795c44e096] WebCore::RenderLayerCompositor::updateBacking()
 [0x7f795c44eba3] WebCore::RenderLayerCompositor::updateLayerCompositingState()
 [0x7f795c420f9e] WebCore::RenderLayer::updateScrollInfoAfterLayout()
 [0x7f795c2fe5d4] WebCore::RenderBlock::updateScrollInfoAfterLayout()
 [0x7f795c2ffc15] WebCore::RenderBlock::layoutBlock()
 [0x7f795c2fe65d] WebCore::RenderBlock::layout()


I considered doing a RenderLayer::updateCanBeStackingContainer() in
RenderLayerCompositor before we call
RenderLayer::useCompositorDrivenAcceleratedScrolling(), but unfortunately all
the RenderLayer instances around there are const, and
RenderLayer::updateCanBeStackingContainer() isn't.

[hartmanng, 2013-08-09 13:28:13]

Ian, please take a look.

[hartmanng, 2013-08-28 21:37:09]

Ian, this should fix the issue we were seeing in the layout test from
https://codereview.chromium.org/22620002/. The problem was that I was avoiding
using m_needsCompositedScrolling for the new codepath (since really _everthing_
needs composited scrolling in the new path as long as it's scrollable).

However, the unintended effect of this was that
didUpdateNeedsCompositedScrolling() didn't always get called (since it only gets
called if the value of m_needsCompositedScrolling changes). This caused the
value of isSelfPainingLayer() to be stale, allowing us to opt out in
RenderLayerCompositor.

The solution is to have the value of m_needsCompositedScrolling depend on
whether we are using the old or new path, by putting a bit more of the logic in
RenderLayer::updateNeedsCompositedScrolling(). This we don't skip over any of
the existing bookkeeping code.

PTAL

[Ian Vollick, 2013-08-29 03:05:00]

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:483: TRACE_EVENT0("comp-scroll",
"RenderLayer::useCompositorDrivenAcceleratedScrolling");
Can you ditch the printf-y trace events for now? I've got another batch of
these, and they're pretty contentious, so maybe we could roll this in with them.

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2091: {
Repeating the switch is a bummer. Maybe we could have a helper bool
RenderLayer::adjustForForceCompositedScrollingMode(bool value)?

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2121: &&
renderer()->view()->frameView()->containsScrollableArea(scrollableArea()));
We've already asserted that
renderer()->view()->frameView()->containsScrollableArea(scrollableArea()))
above. Shouldn't be necessary here.

[hartmanng, 2013-09-05 20:43:58]

Fixed your review comments from last time, and moved the UMA stats (from
https://codereview.chromium.org/23455031) around a bit so they continue to
record the proper information.

There was also a bug that cause the inspector to crash (we weren't always
calling didUpdateNeedsCompositedScrolling() after changing
m_needsToBeStackingContainer) that is also fixed now.

PTAL

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:483: TRACE_EVENT0("comp-scroll",
"RenderLayer::useCompositorDrivenAcceleratedScrolling");
On 2013/08/29 03:05:01, vollick (ooo until 09-02) wrote:
> Can you ditch the printf-y trace events for now? I've got another batch of
> these, and they're pretty contentious, so maybe we could roll this in with
them.

Done.

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2091: {
On 2013/08/29 03:05:01, vollick (ooo until 09-02) wrote:
> Repeating the switch is a bummer. Maybe we could have a helper bool
> RenderLayer::adjustForForceCompositedScrollingMode(bool value)?

Done.

https://codereview.chromium.org/20103002/diff/94001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2121: &&
renderer()->view()->frameView()->containsScrollableArea(scrollableArea()));
On 2013/08/29 03:05:01, vollick (ooo until 09-02) wrote:
> We've already asserted that
> renderer()->view()->frameView()->containsScrollableArea(scrollableArea()))
> above. Shouldn't be necessary here.

Done.

[Ian Vollick, 2013-09-06 02:01:05]

This is looking pretty good! I find the members m_canBeStackingContainer and
m_needsToBeStackingContainer a bit confusing, though. IIUC, the first really
means promoting-me-won't-break-stacking-order. Would it be too awful to change
m_canBeStackingContainer to m_descendantsAreContiguousInStackingOrder or
something similar?

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:481: bool
RenderLayer::useCompositorDrivenAcceleratedScrolling() const
It looks like there's some repeated logic here in hasScrollParent. Perhaps we
could ditch this function and add the m_canBePromoted check there, too?

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2104: if (!scrollParent ||
scrollParent->isStackingContext())
Could this be changed to (scrollParent->needsToBeStackingContainer() &&
!scrollParent->m_canBePromotedToStackingContainerDirty) ?

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2110: if (ancestor->isStackingContext())
Should this be ancestor->stackingContainer()?

[hartmanng, 2013-09-06 20:18:35]

On 2013/09/06 02:01:05, Ian Vollick wrote:
> This is looking pretty good! I find the members m_canBeStackingContainer and
> m_needsToBeStackingContainer a bit confusing, though. IIUC, the first really
> means promoting-me-won't-break-stacking-order. Would it be too awful to change
> m_canBeStackingContainer to m_descendantsAreContiguousInStackingOrder or
> something similar?
> 

Full circle, eh? :)
Done.

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:481: bool
RenderLayer::useCompositorDrivenAcceleratedScrolling() const
On 2013/09/06 02:01:05, Ian Vollick wrote:
> It looks like there's some repeated logic here in hasScrollParent. Perhaps we
> could ditch this function and add the m_canBePromoted check there, too?

Yeah, I guess we can... I thought we needed it from RLC, but apparently not
anymore.

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2104: if (!scrollParent ||
scrollParent->isStackingContext())
On 2013/09/06 02:01:05, Ian Vollick wrote:
> Could this be changed to (scrollParent->needsToBeStackingContainer() &&
> !scrollParent->m_canBePromotedToStackingContainerDirty) ?

That would exclude stacking contexts, are you sure we want to do that?

Also I realize that isStackingContainer() already takes
needsToBeStackingContainer() into account. I think the logic from
useCompositorDriven...() originally relied on canBeStackingContainer() (not
needsToBeStackingContainer()), which is why we needed to check
m_canBePromotedToStackingContainerDirty.

As far as I can tell, just checking scrollParent->isStackingContainer() is
sufficient here.

https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:2110: if (ancestor->isStackingContext())
On 2013/09/06 02:01:05, Ian Vollick wrote:
> Should this be ancestor->stackingContainer()?

yeah, I think so. Done.

[Ian Vollick, 2013-09-06 23:23:41]

This lgtm, but I think it may need to be rebased on the latest version of
https://codereview.chromium.org/23903012/. Is that true?

On 2013/09/06 20:18:35, hartmanng wrote:
> On 2013/09/06 02:01:05, Ian Vollick wrote:
> > This is looking pretty good! I find the members m_canBeStackingContainer and
> > m_needsToBeStackingContainer a bit confusing, though. IIUC, the first really
> > means promoting-me-won't-break-stacking-order. Would it be too awful to
change
> > m_canBeStackingContainer to m_descendantsAreContiguousInStackingOrder or
> > something similar?
> > 
> 
> Full circle, eh? :)
> Done.
Hehe, yep!
> 
>
https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:481: bool
> RenderLayer::useCompositorDrivenAcceleratedScrolling() const
> On 2013/09/06 02:01:05, Ian Vollick wrote:
> > It looks like there's some repeated logic here in hasScrollParent. Perhaps
we
> > could ditch this function and add the m_canBePromoted check there, too?
> 
> Yeah, I guess we can... I thought we needed it from RLC, but apparently not
> anymore.
> 
>
https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:2104: if (!scrollParent ||
> scrollParent->isStackingContext())
> On 2013/09/06 02:01:05, Ian Vollick wrote:
> > Could this be changed to (scrollParent->needsToBeStackingContainer() &&
> > !scrollParent->m_canBePromotedToStackingContainerDirty) ?
> 
> That would exclude stacking contexts, are you sure we want to do that?
> 
> Also I realize that isStackingContainer() already takes
> needsToBeStackingContainer() into account. I think the logic from
> useCompositorDriven...() originally relied on canBeStackingContainer() (not
> needsToBeStackingContainer()), which is why we needed to check
> m_canBePromotedToStackingContainerDirty.
> 
> As far as I can tell, just checking scrollParent->isStackingContainer() is
> sufficient here.
sgtm.
> 
>
https://codereview.chromium.org/20103002/diff/120001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:2110: if (ancestor->isStackingContext())
> On 2013/09/06 02:01:05, Ian Vollick wrote:
> > Should this be ancestor->stackingContainer()?
> 
> yeah, I think so. Done.

[hartmanng, 2013-09-09 14:22:56]

On 2013/09/06 23:23:41, Ian Vollick wrote:
> This lgtm, but I think it may need to be rebased on the latest version of
> https://codereview.chromium.org/23903012/. Is that true?

Yep, done.

[Ian Vollick, 2013-09-09 14:24:53]

On 2013/09/09 14:22:56, hartmanng wrote:
> On 2013/09/06 23:23:41, Ian Vollick wrote:
> > This lgtm, but I think it may need to be rebased on the latest version of
> > https://codereview.chromium.org/23903012/. Is that true?
> 
> Yep, done.

Thanks. slgtm.

[shawnsingh, 2013-09-12 20:26:45]

I was asked to check this just as another pair of eyes - no obvious red flags to
me =)

[commit-bot: I haz the power, 2013-09-13 00:12:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/20103002/145001

[commit-bot: I haz the power, 2013-09-13 01:29:42]

Retried try job too often on win_blink_rel for step(s) webkit_tests,
webkit_unit_tests, weborigin_unittests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[tkent, 2013-09-13 02:30:02]

rubber-stamping lgtm for Source/web/

[hartmanng, 2013-09-13 02:31:32]

thanks!

[commit-bot: I haz the power, 2013-09-13 02:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/20103002/164001

[commit-bot: I haz the power, 2013-09-13 02:35:17]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-09-13 03:22:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/20103002/170001

[commit-bot: I haz the power, 2013-09-13 05:00:33]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-08 17:17:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/20103002/194001

[commit-bot: I haz the power, 2013-10-08 18:24:34]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-08 19:48:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/20103002/214001

[commit-bot: I haz the power, 2013-10-08 23:46:33]

Change committed as 159168