Make sure we set "should flatten transform" correctly.

Source/core/rendering/compositing/CompositedLayerMapping.cpp

 /*
  * Copyright (C) 2009, 2010, 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1078 matching lines @@
 
     if (m_backgroundLayer)
         m_backgroundLayer->setDrawsContent(hasPaintedContent);
 }
 
 void CompositedLayerMapping::updateChildrenTransform()
 {
     if (GraphicsLayer* childTransformLayer = layerForChildrenTransform()) {
         childTransformLayer->setTransform(owningLayer().perspectiveTransform());
         childTransformLayer->setTransformOrigin(FloatPoint3D(childTransformLayer->size().width() * 0.5f, childTransformLayer->size().height() * 0.5f, 0.f));
-        bool hasPerspective = false;
-        if (RenderStyle* style = m_owningLayer.renderer()->style())
-            hasPerspective = style->hasPerspective();
-        if (hasPerspective)
-            childTransformLayer->setShouldFlattenTransform(false);
+    }
 
-        // Note, if the target is the scrolling layer, we need to ensure that the
-        // scrolling content layer doesn't flatten the transform. (It would be nice
-        // if we could apply transform to the scrolling content layer, but that's
-        // too late, we need the children transform to be applied _before_ the
-        // scrolling offset.)
-        if (childTransformLayer == m_scrollingLayer.get())
-            m_scrollingContentsLayer->setShouldFlattenTransform(false);
-    }
+    updateShouldFlattenTransform();
 }
 
 // Return true if the layers changed.
 bool CompositedLayerMapping::updateClippingLayers(bool needsAncestorClip, bool needsDescendantClip)
 {
     bool layersChanged = false;
 
     if (needsAncestorClip) {
         if (!m_ancestorClippingLayer) {
             m_ancestorClippingLayer = createGraphicsLayer(CompositingReasonLayerForAncestorClip);
@@ skipping 24 matching lines @@
 }
 
 bool CompositedLayerMapping::updateChildTransformLayer(bool needsChildTransformLayer)
 {
     bool layersChanged = false;
 
     if (needsChildTransformLayer) {
         if (!m_childTransformLayer) {
             m_childTransformLayer = createGraphicsLayer(CompositingReasonLayerForPerspective);
             m_childTransformLayer->setDrawsContent(false);
-            m_childTransformLayer->setShouldFlattenTransform(false);
             layersChanged = true;
         }
     } else if (m_childTransformLayer) {
         m_childTransformLayer->removeFromParent();
         m_childTransformLayer = nullptr;
         layersChanged = true;
     }
 
+    updateShouldFlattenTransform();

[chrishtr, 2014/07/17 20:07:32]

Is this call needed? Why isn't the other one sufficient?

[Ian Vollick, 2014/07/17 20:30:15]

Good point! I think you're right that this is redundant. I've removed this and
the tests seem to pass (at least the ones that this patch affected).

     return layersChanged;
 }
 
 void CompositedLayerMapping::setBackgroundLayerPaintsFixedRootBackground(bool backgroundLayerPaintsFixedRootBackground)
 {
     m_backgroundLayerPaintsFixedRootBackground = backgroundLayerPaintsFixedRootBackground;
 }
 
 // Only a member function so it can call createGraphicsLayer.
 bool CompositedLayerMapping::toggleScrollbarLayerIfNeeded(OwnPtr<GraphicsLayer>& layer, bool needsLayer, CompositingReasons reason)
@@ skipping 97 matching lines @@
     if ((mode & ApplyToCoreLayers) && mapping->childTransformLayer())
         f(mapping->childTransformLayer());
     if ((mode & ApplyToCoreLayers) && mapping->ancestorClippingLayer())
         f(mapping->ancestorClippingLayer());
     if (((mode & ApplyToCoreLayers) || (mode & ApplyToContentLayers)) && mapping->mainGraphicsLayer())
         f(mapping->mainGraphicsLayer());
     if ((mode & ApplyToCoreLayers) && mapping->clippingLayer())
         f(mapping->clippingLayer());
     if ((mode & ApplyToCoreLayers) && mapping->scrollingLayer())
         f(mapping->scrollingLayer());
+    if ((mode & ApplyToCoreLayers) && mapping->scrollingBlockSelectionLayer())
+        f(mapping->scrollingBlockSelectionLayer());

[chrishtr, 2014/07/17 20:07:32]

Is this an unrelated-ish change?

[Ian Vollick, 2014/07/17 20:30:15]

It isn't, actually. In updateShouldFlattenTransform there are two phases. The
first updates should-flatten-transform as required by preserves-3d, and the
second for perspective. Neither of these phases was operating on the scrolling
block selection layer, but they needed to -- this is the bug this CL is trying
to address. The first phase applies the functor using ApplyToCoreLayers and the
block selection layer needed to be included in that pass. This is what
ApplyToCoreLayers is for, so it's not going to cause the block selection layer
to be included in other, unexpected operations.

[chrishtr, 2014/07/17 20:38:40]

Great. Thanks for the explanation.

     if (((mode & ApplyToCoreLayers) || (mode & ApplyToContentLayers)) && mapping->scrollingContentsLayer())
         f(mapping->scrollingContentsLayer());
     if (((mode & ApplyToCoreLayers) || (mode & ApplyToContentLayers)) && mapping->foregroundLayer())
         f(mapping->foregroundLayer());
 
     if ((mode & ApplyToSquashingLayer) && mapping->squashingLayer())
         f(mapping->squashingLayer());
 
     if (((mode & ApplyToMaskLayers) || (mode & ApplyToContentLayers)) && mapping->maskLayer())
         f(mapping->maskLayer());
@@ skipping 45 matching lines @@
     bool shouldFlatten;
 };
 
 void CompositedLayerMapping::updateShouldFlattenTransform()
 {
     // All CLM-managed layers that could affect a descendant layer should update their
     // should-flatten-transform value (the other layers' transforms don't matter here).
     UpdateShouldFlattenTransformFunctor functor = { !m_owningLayer.shouldPreserve3D() };
     ApplyToGraphicsLayersMode mode = ApplyToCoreLayers;
     ApplyToGraphicsLayers(this, functor, mode);
+
+    // Note, if we apply perspective, we have to set should flatten differently
+    // so that the transform propagates to child layers correctly.
+    if (GraphicsLayer* childTransformLayer = layerForChildrenTransform()) {
+        bool hasPerspective = false;
+        if (RenderStyle* style = m_owningLayer.renderer()->style())
+            hasPerspective = style->hasPerspective();
+        if (hasPerspective)
+            childTransformLayer->setShouldFlattenTransform(false);
+
+        // Note, if the target is the scrolling layer, we need to ensure that the
+        // scrolling content layer doesn't flatten the transform. (It would be nice
+        // if we could apply transform to the scrolling content layer, but that's
+        // too late, we need the children transform to be applied _before_ the
+        // scrolling offset.)
+        if (childTransformLayer == m_scrollingLayer.get()) {
+            m_scrollingContentsLayer->setShouldFlattenTransform(false);
+            m_scrollingBlockSelectionLayer->setShouldFlattenTransform(false);
+        }
+    }
 }
 
 bool CompositedLayerMapping::updateForegroundLayer(bool needsForegroundLayer)
 {
     bool layerChanged = false;
     if (needsForegroundLayer) {
         if (!m_foregroundLayer) {
             m_foregroundLayer = createGraphicsLayer(CompositingReasonLayerForForeground);
             m_foregroundLayer->setDrawsContent(true);
             m_foregroundLayer->setPaintingPhase(GraphicsLayerPaintForeground);
@@ skipping 883 matching lines @@
     } else if (graphicsLayer == m_scrollingBlockSelectionLayer.get()) {
         name = "Scrolling Block Selection Layer";
     } else {
         ASSERT_NOT_REACHED();
     }
 
     return name;
 }
 
 } // namespace WebCore