Promote composited-scrolling layers to stacking contexts.
Created attachment 181397 [details] Patch
Comment on attachment 181397 [details] Patch What are you trying to do here?
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
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.
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
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.
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
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. :(
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
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
Comment on attachment 185037 [details] Patch for landing Clearing flags on attachment: 185037 Committed r140999: <http://trac.webkit.org/changeset/140999>
All reviewed patches have been landed. Closing bug.
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
(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>
(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.
Assertion in bug 108257 seems like it might be related to this patch.