Clear the groupedMapping parameter in layers when removed from the group

Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp

Index: Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
diff --git a/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp b/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
index 4b1ef4df0d2a50e0abae356538608ad271ee726b..c7beadd6a523fe0a302ca5cdfd9b8196fb123aeb 100644
--- a/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
+++ b/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
@@ -2266,36 +2266,40 @@ bool CompositedDeprecatedPaintLayerMapping::updateSquashingLayerAssignment(Depre
     // NOTE: composited bounds are updated elsewhere
     // NOTE: offsetFromLayoutObject is updated elsewhere
 
-    // 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.
-    bool updatedAssignment = false;
+    DeprecatedPaintLayer* formerLayer = nullptr;
     if (nextSquashedLayerIndex < m_squashedLayers.size()) {
-        if (paintInfo.paintLayer != m_squashedLayers[nextSquashedLayerIndex].paintLayer) {
-            compositor()->paintInvalidationOnCompositingChange(squashedLayer);
-            updatedAssignment = true;
-            m_squashedLayers[nextSquashedLayerIndex] = paintInfo;
-        }
+        if (paintInfo.paintLayer == m_squashedLayers[nextSquashedLayerIndex].paintLayer)
+            return false;
+
+        // Track previous entry, but don't remove it yet. We don't want to shuffle the vector.
+        formerLayer = m_squashedLayers[nextSquashedLayerIndex].paintLayer;
+        m_squashedLayers[nextSquashedLayerIndex] = paintInfo;
     } else {
-        compositor()->paintInvalidationOnCompositingChange(squashedLayer);
         m_squashedLayers.append(paintInfo);
-        updatedAssignment = true;
     }
     squashedLayer->setGroupedMapping(this);
-    return updatedAssignment;
+
+    // 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.
+    compositor()->paintInvalidationOnCompositingChange(squashedLayer);
+
+    // Reset the mapping on the previous entry
+    if (formerLayer)
+        formerLayer->setGroupedMapping(nullptr);

[Stephen Chennney, 2015/06/30 19:44:02]

This unfortunately has the side effect of doing some invalidation:

void
DeprecatedPaintLayer::setGroupedMapping(CompositedDeprecatedPaintLayerMapping*
groupedMapping, bool layerBeingDestroyed)
{
    if (groupedMapping == m_groupedMapping)
        return;

    if (!layerBeingDestroyed && m_groupedMapping) {
       
m_groupedMapping->setNeedsGraphicsLayerUpdate(GraphicsLayerUpdateSubtree);
        m_groupedMapping->removeLayerFromSquashingGraphicsLayer(this);
    }
    m_groupedMapping = groupedMapping;
    if (!layerBeingDestroyed && m_groupedMapping)
       
m_groupedMapping->setNeedsGraphicsLayerUpdate(GraphicsLayerUpdateSubtree);
}

I'm am not at all sure we should be doing it, rather than having a shortcut that
just sets the m_groupedMapping to null with no side effect.

+
+    return true;
 }
 
 void CompositedDeprecatedPaintLayerMapping::removeLayerFromSquashingGraphicsLayer(const DeprecatedPaintLayer* layer)
 {
-    size_t layerIndex = kNotFound;
-
-    for (size_t i = 0; i < m_squashedLayers.size(); ++i) {
-        if (m_squashedLayers[i].paintLayer == layer) {
-            layerIndex = i;
+    size_t layerIndex = 0;
+    for (; layerIndex < m_squashedLayers.size(); ++layerIndex) {
+        if (m_squashedLayers[layerIndex].paintLayer == layer) {
             break;
         }
     }
 
-    if (layerIndex == kNotFound)
+    if (layerIndex == m_squashedLayers.size())
         return;
 
     m_squashedLayers.remove(layerIndex);
@@ -2303,8 +2307,15 @@ void CompositedDeprecatedPaintLayerMapping::removeLayerFromSquashingGraphicsLaye
 
 void CompositedDeprecatedPaintLayerMapping::finishAccumulatingSquashingLayers(size_t nextSquashedLayerIndex)
 {
-    // Any additional squashed Layers in the array no longer exist, and removing invalidates the squashingLayer contents.
-    if (nextSquashedLayerIndex < m_squashedLayers.size())
+    // Any additional squashed Layers in the array no longer exist, and removing invalidates the
+    // squashingLayer contents.

[Stephen Chennney, 2015/06/30 19:44:02]

I'm having trouble interpreting this comment. Is it intending to say "Don't call
remove because ti invalidates the contents"? Or is it some cryptic comment about
the m_squashedLayers.remove call?

+    for (size_t i = nextSquashedLayerIndex ; i < m_squashedLayers.size(); ++i) {
+        // Setting to nullptr removes from the vector, via removeLayerFromSquashingGraphicsLayer,
+        // except when the layer is being destroyed.
+        m_squashedLayers[i].paintLayer->setGroupedMapping(nullptr);

[Stephen Chennney, 2015/06/30 19:44:02]

See comment about, might be a real bad idea.

+    }
+
+    if (nextSquashedLayerIndex < m_squashedLayers.size()) {
         m_squashedLayers.remove(nextSquashedLayerIndex, m_squashedLayers.size() - nextSquashedLayerIndex);
 }