[css-grid] Small refactoring in LayoutGrid::layoutPositionedObjects()

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index fc5aa6d6f0a9b1923406f12b6936ff71a7b446b3..333728c46d3428d185ed5c5885f913345cddce32 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -1405,35 +1405,23 @@ void LayoutGrid::layoutPositionedObjects(bool relayoutChildren, PositionedLayout
         }
 
         // FIXME: Detect properly if start/end is auto for inexistent named grid lines.

[cbiesinger, 2015/09/30 14:27:35]

This comment should probably move to offsetAndBreadthForPositionedChild? Or
rather be removed after https://codereview.chromium.org/1375173002/

[Manuel Rego, 2015/09/30 20:53:51]

Yep, you're right, I forgot to move it in this patch.
I'm already removing it in the mentioned patch.

-        bool columnStartIsAuto = child->style()->gridColumnStart().isAuto();
         LayoutUnit columnOffset = LayoutUnit();
         LayoutUnit columnBreadth = LayoutUnit();
-        offsetAndBreadthForPositionedChild(*child, ForColumns, columnStartIsAuto, child->style()->gridColumnEnd().isAuto(), columnOffset, columnBreadth);
-        bool rowStartIsAuto = child->style()->gridRowStart().isAuto();
+        offsetAndBreadthForPositionedChild(*child, ForColumns, columnOffset, columnBreadth);
         LayoutUnit rowOffset = LayoutUnit();
         LayoutUnit rowBreadth = LayoutUnit();
-        offsetAndBreadthForPositionedChild(*child, ForRows, rowStartIsAuto, child->style()->gridRowEnd().isAuto(), rowOffset, rowBreadth);
+        offsetAndBreadthForPositionedChild(*child, ForRows, rowOffset, rowBreadth);
 
         child->setOverrideContainingBlockContentLogicalWidth(columnBreadth);
         child->setOverrideContainingBlockContentLogicalHeight(rowBreadth);
         child->setExtraInlineOffset(columnOffset);
         child->setExtraBlockOffset(rowOffset);
-
-        if (child->parent() == this) {
-            // If column/row start is "auto" the static position has been already set in prepareChildForPositionedLayout().
-            // If column/row start is not "auto" the padding has been already computed in offsetAndBreadthForPositionedChild().
-            DeprecatedPaintLayer* childLayer = child->layer();
-            if (!columnStartIsAuto)
-                childLayer->setStaticInlinePosition(borderStart() + columnOffset);
-            if (!rowStartIsAuto)
-                childLayer->setStaticBlockPosition(borderBefore() + rowOffset);
-        }
     }
 
     LayoutBlock::layoutPositionedObjects(relayoutChildren, info);
 }
 
-void LayoutGrid::offsetAndBreadthForPositionedChild(const LayoutBox& child, GridTrackSizingDirection direction, bool startIsAuto, bool endIsAuto, LayoutUnit& offset, LayoutUnit& breadth)
+void LayoutGrid::offsetAndBreadthForPositionedChild(const LayoutBox& child, GridTrackSizingDirection direction, LayoutUnit& offset, LayoutUnit& breadth)
 {
     ASSERT(child.isHorizontalWritingMode() == isHorizontalWritingMode());
 
@@ -1444,6 +1432,9 @@ void LayoutGrid::offsetAndBreadthForPositionedChild(const LayoutBox& child, Grid
         return;
     }
 
+    bool startIsAuto = (direction == ForColumns) ? child.style()->gridColumnStart().isAuto() : child.style()->gridRowStart().isAuto();
+    bool endIsAuto = (direction == ForColumns) ? child.style()->gridColumnEnd().isAuto() : child.style()->gridRowEnd().isAuto();
+
     GridResolvedPosition firstPosition = GridResolvedPosition(0);
     GridResolvedPosition initialPosition = startIsAuto ? firstPosition : positions->resolvedInitialPosition;
     GridResolvedPosition lastPosition = GridResolvedPosition((direction == ForColumns ? gridColumnCount() : gridRowCount()) - 1);
@@ -1469,6 +1460,15 @@ void LayoutGrid::offsetAndBreadthForPositionedChild(const LayoutBox& child, Grid
     }
 
     offset = start;
+
+    if (child.parent() == this && !startIsAuto) {
+        // If column/row start is "auto" the static position has been already set in prepareChildForPositionedLayout().
+        DeprecatedPaintLayer* childLayer = child.layer();
+        if (direction == ForColumns)
+            childLayer->setStaticInlinePosition(borderStart() + offset);
+        else
+            childLayer->setStaticBlockPosition(borderBefore() + offset);
+    }

[svillar, 2015/09/28 08:09:28]

So now you're doing this before all the child->setXXX calls in
layoutPositionedObjects, are you 100% sure that the order does not matter?

They seem like simple setters but better double check that there are no hidden
side effects.

[Manuel Rego, 2015/09/28 09:49:09]

Yeah, I wouldn't expect other effects as they're just setting different values.

 }
 
 GridCoordinate LayoutGrid::cachedGridCoordinate(const LayoutBox& gridItem) const