Clear the groupedMapping parameter in layers when removed from the group

Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.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 2248 matching lines @@
     return pixelSnappedIntRect(bounds);
 }
 
 bool CompositedDeprecatedPaintLayerMapping::updateSquashingLayerAssignment(DeprecatedPaintLayer* squashedLayer, const DeprecatedPaintLayer& owningLayer, size_t nextSquashedLayerIndex)
 {
     GraphicsLayerPaintInfo paintInfo;
     paintInfo.paintLayer = squashedLayer;
     // NOTE: composited bounds are updated elsewhere
     // NOTE: offsetFromLayoutObject is updated elsewhere
 
+    DeprecatedPaintLayer* formerLayer = nullptr;
+    if (nextSquashedLayerIndex < m_squashedLayers.size()) {
+        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 {
+        m_squashedLayers.append(paintInfo);
+    }
+    squashedLayer->setGroupedMapping(this);
+
     // 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;
-    if (nextSquashedLayerIndex < m_squashedLayers.size()) {
-        if (paintInfo.paintLayer != m_squashedLayers[nextSquashedLayerIndex].paintLayer) {
-            compositor()->paintInvalidationOnCompositingChange(squashedLayer);
-            updatedAssignment = true;
-            m_squashedLayers[nextSquashedLayerIndex] = paintInfo;
-        }
-    } else {
-        compositor()->paintInvalidationOnCompositingChange(squashedLayer);
-        m_squashedLayers.append(paintInfo);
-        updatedAssignment = true;
-    }
-    squashedLayer->setGroupedMapping(this);
-    return updatedAssignment;
+    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);
 }
 
 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);
 }
 
 String CompositedDeprecatedPaintLayerMapping::debugName(const GraphicsLayer* graphicsLayer)
 {
     String name;
     if (graphicsLayer == m_graphicsLayer.get()) {
         name = m_owningLayer.debugName();
     } else if (graphicsLayer == m_squashingContainmentLayer.get()) {
         name = "Squashing Containment Layer";
@@ skipping 30 matching lines @@
     } else if (graphicsLayer == m_scrollingBlockSelectionLayer.get()) {
         name = "Scrolling Block Selection Layer";
     } else {
         ASSERT_NOT_REACHED();
     }
 
     return name;
 }
 
 } // namespace blink