Bug 106142 – Promote composited-scrolling layers to stacking containers.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Glenn Hartmann]

Promote composited-scrolling layers to stacking contexts.

[Glenn Hartmann]

Created attachment 181397 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 181397 [details]
Patch

What are you trying to do here?

[Build Bot]

Comment on attachment 181397 [details]
Patch

Attachment 181397 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15686371

New failing tests:
compositing/overflow/automatically-opt-into-composited-scrolling.html

[WebKit Review Bot]

Comment on attachment 181397 [details]
Patch

Attachment 181397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15706514

New failing tests:
platform/chromium/virtual/softwarecompositing/overflow/automatically-opt-into-composited-scrolling.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/automatically-opt-into-composited-scrolling.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-scroll.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-scroll-with-touch-no-overflow.html
compositing/overflow/automatically-opt-into-composited-scrolling.html

Created attachment 182491 [details]
Patch

(In reply to comment #2)
> (From update of attachment 181397 [details])
> What are you trying to do here?

With the latest patch, there's a more detailed explanation of the intent of the patch in the ChangeLog. If it's insufficient or incorrect, please let me know.

[Build Bot]

Comment on attachment 182491 [details]
Patch

Attachment 182491 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15838221

New failing tests:
compositing/overflow/composited-scrolling-creates-a-stacking-context.html

[WebKit Review Bot]

Comment on attachment 182491 [details]
Patch

Attachment 182491 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15845249

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/overflow/scrollbars-with-clipped-owner.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-scroll.html

Comment on attachment 182491 [details]
Patch

platform/chromium/virtual/gpu/compositedscrolling/overflow/scrollbars-with-clipped-owner.html looks like a real failure. Removing r? while I investigate.

[Simon Fraser (smfr)]

Comment on attachment 182491 [details]
Patch

Ugh, so much additional complexity. This would be so much simpler if you could just whack style->zIndex() when you know that you can safely propagate this overflow element into a stacking context. What's the downside of doing this based simply on looking at style, not whether it actually overlows now?

Created attachment 182623 [details]
Patch

(In reply to comment #10)
> (From update of attachment 182491 [details])
> Ugh, so much additional complexity. This would be so much simpler if you could just whack style->zIndex() when you know that you can safely propagate this overflow element into a stacking context. What's the downside of doing this based simply on looking at style, not whether it actually overlows now?

I was hoping to do this, too, but the problem is that I use the value of isStackingContext when computing m_needsCompositedScrolling. So when computing m_needsCompositedScrolling, rather than asking if a layer is a stacking context I really need to ask "would you be a stacking context irrespective of your composited scrolling status?" If I promote to a stacking context by mutating the style, I'm no longer able to ask this question because the composited-scrolling effect is baked in.

Also in this patch:
 - Added the missing mac test expectation.
 - Updated the chromium TestExpectations for the test that will need rebaselining.
 - Removed the optimization in RenderLayer::updateCompositedLayersAfterScroll. Should be in its own patch anyhow.

Created attachment 183032 [details]
Patch

[Elliott Sprehn]

Comment on attachment 183032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183032&action=review

> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:152
> +    if (renderLayer->treatAsAStackingContext()) {

nit: having the word "as" in there twice (even though one isn't really) makes reading this method name really hard to read. :(

[Ryosuke Niwa]

Comment on attachment 183032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183032&action=review

>> Source/WebCore/inspector/InspectorLayerTreeAgent.cpp:152
>> +    if (renderLayer->treatAsAStackingContext()) {
> 
> nit: having the word "as" in there twice (even though one isn't really) makes reading this method name really hard to read. :(

We don't normally include articles in function and variable names. You can call it treatAsSingleStackingContext if singularity is important.

Created attachment 183084 [details]
Patch

This patch introduces the stackingContainer concept.

To hopefully simplify the job for any potential reviewers, here's a guide to the latest patch.

1) It introduces the idea of a "stacking container." A stacking container is treated just like a stacking context, but it isn't one in the strict, CSS sense.

2) In most places where we'd referred to 'stacking context', we really meant 'stacking container'. These name changes make up the bulk of the patch.

3) RenderLayers that needCompositedScrolling are now considered stacking containers (the whole point of introducing the stacking container concept was so that it could be extended to include these layers too).

4) The code in RenderLayer that determines the value of needsCompositedScrolling (updateDescendantsAreContiguousInStackingOrder, updateNeedsCompositedScrolling, etc), obviously can't depend on the value of isStackingContainer since isStackingContainer depends on needsCompositedScrolling, so all that code has been modified to use isStackingContext instead. In particular, the code that builds the z-order lists needed to be generalized so that we could use isStackingContext when building the layer lists for updateDescendantsAreContiguousInStackingOrder, and isStackingContainer when building the 'real' z-order lists.

Created attachment 184388 [details]
Patch

[James Robinson]

Comment on attachment 184388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184388&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1926
> +        RenderLayer* sc = parent();

no abbreviations! 'sc' needs a better name

> Source/WebCore/rendering/RenderLayer.cpp:1933
> +        while (sc && !sc->isStackingContext())
> +            sc = sc->parent();
> +        
> +        if (sc) {
> +            sc->dirtyZOrderLists();
> +            sc->dirtyNormalFlowList();
> +        }

why is this different from dirtyStackingContainerZOrderLists() ? if this RenderLayer is in a stacking container that's not a stacking context's lists, wouldn't we have to dirty those?

> Source/WebCore/rendering/RenderLayer.cpp:1935
> +        compositor()->setCompositingLayersNeedRebuild();
> +        compositor()->setShouldReevaluateCompositingAfterLayout();

why do you need both of these?

Created attachment 184804 [details]
Patch

(In reply to comment #18)
  > (From update of attachment 184388 [details])
  > View in context: https://bugs.webkit.org/attachment.cgi?id=184388&action=review
  > 
  > > Source/WebCore/rendering/RenderLayer.cpp:1926
  > > +        RenderLayer* sc = parent();
  > 
  > no abbreviations! 'sc' needs a better name
  > 
  > > Source/WebCore/rendering/RenderLayer.cpp:1933
  > > +        while (sc && !sc->isStackingContext())
  > > +            sc = sc->parent();
  > > +        
  > > +        if (sc) {
    > > +            sc->dirtyZOrderLists();
    > > +            sc->dirtyNormalFlowList();
    > > +        }
    > 
    > why is this different from dirtyStackingContainerZOrderLists() ? if this RenderLayer is in a stacking container that's not a stacking context's lists, wouldn't we have to dirty those?
Good point! I've switched this to dirtyStackingContainerZOrderLists(). This also fixes the variable naming problem you mentioned.
    > 
    > > Source/WebCore/rendering/RenderLayer.cpp:1935
    > > +        compositor()->setCompositingLayersNeedRebuild();
    > > +        compositor()->setShouldReevaluateCompositingAfterLayout();
    > 
    > why do you need both of these?
Unless I'm mistaken, if I just include the first, we will early out in RenderLayerCompositor::updateCompositingLayers if we're not compositing (it will think that there's no work to do), and if I include just the second, we won't rebuild the layer tree if we're already compositing, and we'll need to if we've dirtied a bunch of layer lists.

Created attachment 185037 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185037 [details]
Patch for landing

Clearing flags on attachment: 185037

Committed r140999: <http://trac.webkit.org/changeset/140999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Vujovic]

I'm seeing a possibly related compile failure on the Mac build bots:

Undefined symbols for architecture x86_64:
  "__ZNK7WebCore11RenderLayer24needsCompositedScrollingEv", referenced from:
      __ZNK7WebCore11RenderLayer19isStackingContainerEv in WebRenderLayer.o

http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/7856/steps/compile-webkit/logs/stdio

[Joseph Pecoraro]

(In reply to comment #23)
> I'm seeing a possibly related compile failure on the Mac build bots:
> 
> Undefined symbols for architecture x86_64:
>   "__ZNK7WebCore11RenderLayer24needsCompositedScrollingEv", referenced from:
>       __ZNK7WebCore11RenderLayer19isStackingContainerEv in WebRenderLayer.o
> 
> http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/7856/steps/compile-webkit/logs/stdio

Addressed with a build fix: <http://trac.webkit.org/changeset/141012>

[Max Vujovic]

(In reply to comment #24)
> (In reply to comment #23)
> > I'm seeing a possibly related compile failure on the Mac build bots:
> > 
> > Undefined symbols for architecture x86_64:
> >   "__ZNK7WebCore11RenderLayer24needsCompositedScrollingEv", referenced from:
> >       __ZNK7WebCore11RenderLayer19isStackingContainerEv in WebRenderLayer.o
> > 
> > http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/7856/steps/compile-webkit/logs/stdio
> 
> Addressed with a build fix: <http://trac.webkit.org/changeset/141012>

Thanks! The build fix worked locally.

[Jussi Kukkonen (jku)]

Assertion in bug 108257 seems like it might be related to this patch.