Land layer squashing behind a flag

Source/core/rendering/CompositedLayerMapping.cpp

Index: Source/core/rendering/CompositedLayerMapping.cpp
diff --git a/Source/core/rendering/CompositedLayerMapping.cpp b/Source/core/rendering/CompositedLayerMapping.cpp
index 8a1fae7e67f12370dffa5a5090dae4d4909e4d68..0a0c93ff788347e06c812388dc2d5fcfb8148de5 100644
--- a/Source/core/rendering/CompositedLayerMapping.cpp
+++ b/Source/core/rendering/CompositedLayerMapping.cpp
@@ -175,6 +175,13 @@ CompositedLayerMapping::CompositedLayerMapping(RenderLayer* layer)
 
 CompositedLayerMapping::~CompositedLayerMapping()
 {
+    // FIXME: this almost certainly needs to go somewhere more appropriate so it occurs
+    // when things change as well as things getting destroyed.

[Ian Vollick, 2013/12/02 21:17:05]

I wonder if it can be driven from RLC::rebuildCompLayerTree?

[shawnsingh, 2013/12/05 17:55:24]

This FIXME should be removed.  I've convinced myself that it's appropriate to
remove references of this CLM on renderLayers when the CLM is getting destroyed,
and any other changes should rely on the full compositing update to recompute
squashing assignments.  So this code belongs either here or in
assignLayersToBackings where the CLM is de-allocated.

Also, the condition 2 lines below this is sligtly incorrect.  In the new patch I
fixed it it so that we only setGroupedMapping(0) if the renderLayer referred to
this particular CLM that's being destroyed.

+    for (size_t i = 0; i < m_squashedLayers.size(); ++i) {
+        if (m_squashedLayers[i].renderLayer->compositingState() == PaintsIntoGroupedBacking)
+            m_squashedLayers[i].renderLayer->setGroupedMapping(0);
+    }
+
     updateClippingLayers(false, false);
     updateOverflowControlsLayers(false, false, false);
     updateForegroundLayer(false);
@@ -182,6 +189,7 @@ CompositedLayerMapping::~CompositedLayerMapping()
     updateMaskLayer(false);
     updateClippingMaskLayers(false);
     updateScrollingLayers(false);
+    updateSquashingLayers(false);
     destroyGraphicsLayers();
 }
 
@@ -457,6 +465,9 @@ bool CompositedLayerMapping::updateGraphicsLayerConfiguration()
     updateScrollParent(scrollParent);
     updateClipParent(m_owningLayer->clipParent());
 
+    if (updateSquashingLayers(!m_squashedLayers.isEmpty()))
+        layerConfigChanged = true;
+
     if (layerConfigChanged)
         updateInternalHierarchy();
 
@@ -566,6 +577,7 @@ void CompositedLayerMapping::updateGraphicsLayerGeometry()
     // We compute everything relative to the enclosing compositing layer.
     IntRect ancestorCompositingBounds;
     if (compAncestor) {
+        // FIXME: is it correct to fix the ASSERT by adding the squashing condition here, too?

[Ian Vollick, 2013/12/02 21:17:05]

Yeah, I think so. It should be valid for compAncestor to be squashed.

[shawnsingh, 2013/12/05 17:55:24]

Actually I've convinced myself this FIXME is bogus, too.  compAncestor should
definitely assert that it has it's own compositing mapping here.  Another way to
see this - a squashing layer should never be a compositedAncestor for other
composited content.  It would always be a leaf node in the composited layer
tree.

         ASSERT(compAncestor->hasCompositedLayerMapping());
         ancestorCompositingBounds = pixelSnappedIntRect(compAncestor->compositedLayerMapping()->compositedBounds());
     }
@@ -780,6 +792,44 @@ void CompositedLayerMapping::updateGraphicsLayerGeometry()
         }
     }
 
+    if (m_squashingLayer) {

[Ian Vollick, 2013/12/02 21:17:05]

IIUC, we ultimately want to postpone geometry computation for squashing layers
until all squashed layers have finished doing their geometry stuff, right? What
would you think of breaking this out into a separate fn. We could update
geometry as if there were no squashing first, and then do a pass over the
squashing layers to update stuff for squashing? Perhaps that would be too slow?
Or wrong? Both?

[shawnsingh, 2013/12/05 17:55:24]

Can you please elaborate what geometry stuff is unfinished by this point?  I'm
not seeing it.  This code gets run after all layers have been assigned to the
appropriate backings (i.e. after the CLM m_squashingLayer knows all
squashedLayers that will paint into it).

I do agree we should eventually put this code outside of
updateGraphicsLayerGeometry, but it seems the appropriate time to pull this out
would be when we pull squashingLayer out entirely.

[Ian Vollick, 2013/12/06 21:11:30]

I was incorrectly worried that calculateCompositedLayerBounds would return the
wrong thing until the updateGraphicsLayerGeometry was done.

+        ASSERT(compositor()->isLayerSquashingEnabled());
+
+        IntRect totalSquashBounds;
+        for (size_t i = 0; i < m_squashedLayers.size(); ++i) {
+            IntRect squashedBounds = compositor()->calculateCompositedBounds(m_squashedLayers[i].renderLayer, m_squashedLayers[i].renderLayer);
+
+            // Store the composited bounds before applying the offset.
+            // FIXME: do we need to? is it better for some reason?

[Ian Vollick, 2013/12/02 21:17:05]

We'll always be able to move it back, if necessary since we know
offsetFromBackingRoot, right? If so, then I don't think it matters too much.

+            m_squashedLayers[i].compositedBounds = squashedBounds;
+
+            squashedBounds.move(m_squashedLayers[i].offsetFromBackingRoot);
+            totalSquashBounds.unite(squashedBounds);
+        }
+
+        IntPoint squashLayerPosition;
+        if (m_ancestorClippingLayer) {

[Ian Vollick, 2013/12/02 21:17:05]

Ancestor transform layer's gonna have to be dealt with here, too, I think. Dunno
if that warrants a FIXME, though. That patch may still fail.

+            squashLayerPosition = IntPoint(m_ancestorClippingLayer->position().x() + totalSquashBounds.location().x(),
+                m_ancestorClippingLayer->position().y() + totalSquashBounds.location().y());
+        } else {
+            squashLayerPosition = IntPoint(m_graphicsLayer->position().x() + totalSquashBounds.location().x(),
+                m_graphicsLayer->position().y() + totalSquashBounds.location().y());
+        }
+
+        m_squashingLayer->setPosition(squashLayerPosition);
+        m_squashingLayer->setSize(totalSquashBounds.size());

[Ian Vollick, 2013/12/02 21:17:05]

It occurs to me that perspective depends on layer size (it has an effect on the
perspective origin). I don't think we should squash layers w/
-webkit-perspective. If this is true, is it already on your radar?

[shawnsingh, 2013/12/05 17:55:24]

Yeah, I am planning to add a canBeSquashed() function that returns false in the
various conditions we want to skip squashing.

+
+        // Now that the position of the squashing layer is known, update the offsets for each squashed RenderLayer.
+        // FIXME: find a cleaner way to compute renderer offsets, hopefully that supports transforms, too?

[Ian Vollick, 2013/12/02 21:17:05]

I kinda think we should punt on squashing layers w/ 3d transforms forever.

[shawnsingh, 2013/12/05 17:55:24]

3d transforms would always be composited... ?
This FIXME refers to 2d transforms that do need to be supported.  There are just
2-3 known bugs that should be straightforward to fix to get 2d transforms
squashed properly, I already have the test cases for it, too.

[Ian Vollick, 2013/12/06 21:11:30]

(facepalm). Of course. Sorry, carry on..

+        for (size_t i = 0; i < m_squashedLayers.size(); ++i) {
+            m_squashedLayers[i].offsetFromRenderer = IntSize(-m_squashedLayers[i].offsetFromBackingRoot.width() - m_graphicsLayer->position().x() + m_squashingLayer->position().x(),
+                -m_squashedLayers[i].offsetFromBackingRoot.height() - m_graphicsLayer->position().y() + m_squashingLayer->position().y());
+
+            // FIXME: find a better design to avoid this redundant value
+            m_squashedLayers[i].renderLayer->setOffsetFromSquashingLayerOrigin(m_squashedLayers[i].offsetFromRenderer);
+        }
+    }
+
     if (m_owningLayer->scrollableArea())
         m_owningLayer->scrollableArea()->positionOverflowControls();
 
@@ -853,6 +903,19 @@ void CompositedLayerMapping::updateInternalHierarchy()
         m_layerForScrollCorner->removeFromParent();
         m_graphicsLayer->addChild(m_layerForScrollCorner.get());
     }
+
+    // The squashing containment layer, if it exists, becomes a no-op parent.
+    if (m_squashingLayer) {

[Ian Vollick, 2013/12/02 21:17:05]

Usually there's a bit here where we remove all children from a layer before
adding new children in. Is that not necessary for the squashing containment
layer?

[shawnsingh, 2013/12/05 17:55:24]

I think I just missed it.  Added to new patch, thanks for catching!

+        ASSERT(compositor()->isLayerSquashingEnabled());
+        ASSERT(m_squashingContainmentLayer);
+
+        if (m_ancestorClippingLayer)
+            m_squashingContainmentLayer->addChild(m_ancestorClippingLayer.get());
+        else
+            m_squashingContainmentLayer->addChild(m_graphicsLayer.get());
+
+        m_squashingContainmentLayer->addChild(m_squashingLayer.get());
+    }
 }
 
 void CompositedLayerMapping::updateContentsRect(bool isSimpleContainer)
@@ -1179,6 +1242,8 @@ bool CompositedLayerMapping::updateScrollingLayers(bool needsScrollingLayers)
 
 void CompositedLayerMapping::updateScrollParent(RenderLayer* scrollParent)
 {
+    // FIXME: Either the asserts or the code in his function are broken for squashing. (I think it's the asserts?)

[Ian Vollick, 2013/12/02 21:17:05]

It's broken. There are three layers that might be the 'top' of the CLM's
hierarchy. The ancestor clip layer, graphics layer, now squashing containment
layer, and eventually ancestor transform layer. We need to do
scrollingCoordinator->updateScrollParentForGraphicsLayer(notTheTopOfTheHierarchy,
0) for all non-NULL layers that aren't at the top, and
sc->uSPFGL->(topOfTheHierarchy, scrollParent) for the top. This fn is brittle
and badly written (sorry).

[shawnsingh, 2013/12/05 17:55:24]

Please let me know if new patch does it properly =)

+
     if (ScrollingCoordinator* scrollingCoordinator = scrollingCoordinatorFromLayer(m_owningLayer)) {
         if (m_ancestorClippingLayer) {
             ASSERT(childForSuperlayers() == m_ancestorClippingLayer.get());
@@ -1200,6 +1265,50 @@ void CompositedLayerMapping::updateClipParent(RenderLayer* clipParent)
         scrollingCoordinator->updateClipParentForGraphicsLayer(m_graphicsLayer.get(), clipParent);
 }
 
+bool CompositedLayerMapping::updateSquashingLayers(bool needsSquashingLayers)
+{
+    bool layersChanged = false;
+
+    if (needsSquashingLayers) {
+        ASSERT(compositor()->isLayerSquashingEnabled());
+
+        if (!m_squashingLayer) {
+            m_squashingLayer = createGraphicsLayer(CompositingReasonOverlap);
+            m_squashingLayer->setDrawsContent(true);
+            m_squashingLayer->setNeedsDisplay();
+            layersChanged = true;
+        }
+
+        if (!m_squashingContainmentLayer) {

[trchen, 2013/12/02 22:56:55]

This can merge with the if-block above, with
ASSERT(!m_squashingContainmentLayer).

Having two ifs may seem more robust, but gives reader the wrong impression that
either layer can ever exist singly.

+            // FIXME: containment layer needs a new CompositingReason, CompositingReasonOverlap is not appropriate.
+            m_squashingContainmentLayer = createGraphicsLayer(CompositingReasonOverlap);
+            // FIXME: reflections should force transform-style to be flat in the style: https://bugs.webkit.org/show_bug.cgi?id=106959
+            bool preserves3D = renderer()->style()->transformStyle3D() == TransformStyle3DPreserve3D && !renderer()->hasReflection();

[Ian Vollick, 2013/12/02 21:17:05]

Can we write a CLM::preserves3D() helper fn? I find we're rewriting this snippet
often in a couple of places.

[trchen, 2013/12/02 22:56:55]

Couldn't we setPreserves3D(true) always? Squashing containment layer is supposed
to be a no-op layer. If the element wants to flatten stuffs, the stuffs will be
flattened in m_graphicsLayer already.
(Actually I feel the default value for GraphicsLayer::m_preserves3D should be
true, which is closer to no-op.)

[shawnsingh, 2013/12/05 17:55:24]

I would prefer to call it CLM::owningLayerPreserves3D()

we cannot just setPreserves3D(true).  This would be creating a random 3d
rendering context where one should not exist, and layers would incorrectly sort
against each other.  The example of this is shown in the attached test case here
- crbug.com/322628

I'm going to wait until Ian's work about this is done, and rebase correctly on
top of that.

+            m_squashingContainmentLayer->setPreserves3D(preserves3D);
+            layersChanged = true;
+        }
+
+        ASSERT(m_squashingLayer && m_squashingContainmentLayer);
+    } else {
+        if (m_squashingLayer) {
+            m_squashingLayer->removeFromParent();

[trchen, 2013/12/02 22:56:55]

No need to explicitly removeFromParent. The destructor guarantees it.

[shawnsingh, 2013/12/05 17:55:24]

The style in the rest of this file does explicitly use removeFromParent() - I
would prefer to keep it uniform with the rest of the file, and we could discuss
this cleanup separately...

+            m_squashingLayer = nullptr;
+            layersChanged = true;
+            // FIXME: do we need to invalidate something here?
+        }
+
+        if (m_squashingContainmentLayer) {
+            m_squashingContainmentLayer->removeFromParent();

[trchen, 2013/12/02 22:56:55]

Ditto

+            m_squashingContainmentLayer = nullptr;
+            layersChanged = true;
+        }
+
+        ASSERT(!m_squashingLayer && !m_squashingContainmentLayer);
+    }
+
+    return layersChanged;
+}
+
 GraphicsLayerPaintingPhase CompositedLayerMapping::paintingPhaseForPrimaryLayer() const
 {
     unsigned phase = 0;
@@ -1541,6 +1650,9 @@ GraphicsLayer* CompositedLayerMapping::parentForSublayers() const
 
 GraphicsLayer* CompositedLayerMapping::childForSuperlayers() const
 {
+    if (m_squashingContainmentLayer)
+        return m_squashingContainmentLayer.get();
+
     if (m_ancestorClippingLayer)
         return m_ancestorClippingLayer.get();
 
@@ -1708,14 +1820,20 @@ void CompositedLayerMapping::doPaintTask(GraphicsLayerPaintInfo& paintInfo, Grap
     paintInfo.renderLayer->renderer()->assertSubtreeIsLaidOut();
 #endif
 
-    // FIXME: GraphicsLayers need a way to split for RenderRegions.
-    LayerPaintingInfo paintingInfo(paintInfo.renderLayer, dirtyRect, PaintBehaviorNormal, LayoutSize());
-    paintInfo.renderLayer->paintLayerContents(context, paintingInfo, paintFlags);
+    if (paintInfo.renderLayer->compositingState() != PaintsIntoGroupedBacking) {
+        // FIXME: GraphicsLayers need a way to split for RenderRegions.
+        LayerPaintingInfo paintingInfo(paintInfo.renderLayer, dirtyRect, PaintBehaviorNormal, LayoutSize());
+        paintInfo.renderLayer->paintLayerContents(context, paintingInfo, paintFlags);
 
-    ASSERT(!paintInfo.isBackgroundLayer || paintFlags & PaintLayerPaintingRootBackgroundOnly);
+        ASSERT(!paintInfo.isBackgroundLayer || paintFlags & PaintLayerPaintingRootBackgroundOnly);
 
-    if (paintInfo.renderLayer->containsDirtyOverlayScrollbars())
-        paintInfo.renderLayer->paintLayerContents(context, paintingInfo, paintFlags | PaintLayerPaintingOverlayScrollbars);
+        if (paintInfo.renderLayer->containsDirtyOverlayScrollbars())
+            paintInfo.renderLayer->paintLayerContents(context, paintingInfo, paintFlags | PaintLayerPaintingOverlayScrollbars);
+    } else {
+        ASSERT(compositor()->isLayerSquashingEnabled());
+        LayerPaintingInfo paintingInfo(paintInfo.renderLayer, dirtyRect, PaintBehaviorNormal, LayoutSize());
+        paintInfo.renderLayer->paintLayer(context, paintingInfo, paintFlags);
+    }
 
     ASSERT(!paintInfo.renderLayer->m_usedTransparency);
 
@@ -1762,6 +1880,10 @@ void CompositedLayerMapping::paintContents(const GraphicsLayer* graphicsLayer, G
 
         // We have to use the same root as for hit testing, because both methods can compute and cache clipRects.
         doPaintTask(paintInfo, &context, clip);

[trchen, 2013/12/02 22:56:55]

Do we have to skip painting here if we're squashed into someone else's layer?

[shawnsingh, 2013/12/05 17:55:24]

We will guarantee not to reach this code when the squashing GraphicsLayer is
asking to paint.  And we are also guaranteed that a squashed RenderLayer will
never own it's own OwnPtr compositedLayerMapping.  (This is asserted in
compositingState() accessor, and possibly other places)  

So that means we would never ask a squashed RenderLayer to paint in this chunk
of code.

That said, there is a repaint bug that should be straightforward for me to solve
where i'm accidentally double-painting squashed contents.  I believe that's a
bug in RenderLayer.cpp paint() entry points.

+    } else if (graphicsLayer == m_squashingLayer.get()) {
+        ASSERT(compositor()->isLayerSquashingEnabled());
+        for (size_t i = 0; i < m_squashedLayers.size(); ++i)
+            doPaintTask(m_squashedLayers[i], &context, clip);

[Ian Vollick, 2013/12/02 21:17:05]

Cool.

     } else if (graphicsLayer == layerForHorizontalScrollbar()) {
         paintScrollbar(m_owningLayer->scrollableArea()->horizontalScrollbar(), context, clip);
     } else if (graphicsLayer == layerForVerticalScrollbar()) {
@@ -1957,6 +2079,45 @@ void CompositedLayerMapping::setCompositedBounds(const IntRect& bounds)
     m_compositedBounds = bounds;
 }
 
+void CompositedLayerMapping::addRenderLayerToSquashingGraphicsLayer(RenderLayer* layer, IntSize offsetFromTargetBacking, size_t nextSquashedLayerIndex)
+{
+    ASSERT(compositor()->isLayerSquashingEnabled());
+
+    GraphicsLayerPaintInfo paintInfo;
+    paintInfo.renderLayer = layer;
+    // NOTE: composited bounds are updated elsewhere
+    // NOTE: offsetFromRenderer is updated elsewhere
+    paintInfo.offsetFromBackingRoot = offsetFromTargetBacking;
+    paintInfo.paintingPhase = GraphicsLayerPaintAllWithOverflowClip;
+    paintInfo.isBackgroundLayer = false;
+
+    // Change tracking on squashing layers: at the first sign of something changed, just invalidate the layer.
+    // FIXME: Perhaps we can find a tighter more clever mechanism later.

[Ian Vollick, 2013/12/02 21:17:05]

Makes sense, but for now, this seems ok to me, too.

+    if (nextSquashedLayerIndex < m_squashedLayers.size()) {
+        if (m_squashedLayers[nextSquashedLayerIndex].renderLayer != layer) {
+            m_squashedLayers[nextSquashedLayerIndex] = paintInfo;
+            m_squashingLayer->setNeedsDisplay();

[trchen, 2013/12/02 22:56:55]

Why null checking is needed below but not here?

[shawnsingh, 2013/12/05 17:55:24]

Thanks for catching this - null check is needed in both places.

+        }
+    } else {
+        m_squashedLayers.append(paintInfo);
+        if (m_squashingLayer)
+            m_squashingLayer->setNeedsDisplay();
+    }
+    layer->setGroupedMapping(this);
+}
+
+void CompositedLayerMapping::finishAccumulatingSquashingLayers(size_t nextSquashedLayerIndex)
+{
+    ASSERT(compositor()->isLayerSquashingEnabled());
+
+    // Any additional squashed RenderLayers in the array no longer exist, and removing invalidates the squashingLayer contents.
+    if (nextSquashedLayerIndex < m_squashedLayers.size()) {
+        m_squashedLayers.remove(nextSquashedLayerIndex, m_squashedLayers.size() - nextSquashedLayerIndex);
+        if (m_squashingLayer)
+            m_squashingLayer->setNeedsDisplay();
+    }
+}
+
 CompositingLayerType CompositedLayerMapping::compositingLayerType() const
 {
     if (m_graphicsLayer->hasContentsLayer())