[CSS Grid Layout] Avoid inserting grid items twice in the grid

Source/core/rendering/RenderGrid.cpp

 /*
  * Copyright (C) 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 189 matching lines @@
 {
 }
 
 void RenderGrid::addChild(RenderObject* newChild, RenderObject* beforeChild)
 {
     RenderBlock::addChild(newChild, beforeChild);
 
     if (gridIsDirty())
         return;
 
+    // If the new requested beforeChild is not one of our children is because it's wrapped by an anonymous container. In
+    // those cases addChild will be called twice for the newChild, one with the initial beforeChild and another one with
+    // its parent (the anonymous block container). See RenderBlock::addChildIgnoringAnonymousColumnBlocks()
+    if (beforeChild && beforeChild->parent() != this) {
+        ASSERT(beforeChild->parent()->isAnonymous());
+        dirtyGrid();

[Julien - ping for review, 2014/06/25 21:25:08]

Do we really need to invalidate the grid in this case if we assume we are going
to get called for the real children?

Also other renderers (tables, blocks) have to deal with similar issues and they
end up splitting the renderer in 2. I think we may need some similar logic here
(see splitAnonymousBoxesAroundChild).

[svillar, 2014/06/26 09:45:53]

Good point, should be safe to just return indeed.
Hmm let's take a look.

+        return;
+    }
+
     if (!newChild->isBox()) {
         dirtyGrid();
         return;
     }
 
     if (style()->gridAutoFlow() != AutoFlowNone) {
         // The grid needs to be recomputed as it might contain auto-placed items that will change their position.
         dirtyGrid();
         return;
     }
 
     RenderBox* newChildBox = toRenderBox(newChild);
     OwnPtr<GridSpan> rowPositions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), *newChildBox, ForRows);
     OwnPtr<GridSpan> columnPositions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), *newChildBox, ForColumns);
     if (!rowPositions || !columnPositions) {
         // The new child requires the auto-placement algorithm to run so we need to recompute the grid fully.
         dirtyGrid();
         return;
     } else {
         insertItemIntoGrid(newChildBox, GridCoordinate(*rowPositions, *columnPositions));
         addChildToIndexesMap(newChildBox);

[Manuel Rego, 2014/06/25 20:13:11]

Probably we could add an ASSERT here, similar to the one in 
RenderGrid::addChildToIndexesMap(), to be sure that the map doesn't
contain the element yet. If this is called twice again at some point
it'll help to detect it.

[Julien - ping for review, 2014/06/25 21:25:08]

I would make it a RELEASE_ASSERT as it seems like a potential for a disaster
down the road so we may as well catch it earlier.

     }
 }
 
 void RenderGrid::addChildToIndexesMap(RenderBox* child)
 {
     ASSERT(!m_gridItemsIndexesMap.contains(child));
     RenderBox* sibling = child->nextSiblingBox();
     bool lastSibling = !sibling;
 
     if (lastSibling)
@@ skipping 1008 matching lines @@
     if (isOutOfFlowPositioned())
         return "RenderGrid (positioned)";
     if (isAnonymous())
         return "RenderGrid (generated)";
     if (isRelPositioned())
         return "RenderGrid (relative positioned)";
     return "RenderGrid";
 }
 
 } // namespace WebCore