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

third_party/WebKit/Source/core/layout/LayoutGrid.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 1386 matching lines @@
         return;
 
     bool containerHasHorizontalWritingMode = isHorizontalWritingMode();
     for (auto* child : *positionedDescendants) {
         bool hasOrthogonalWritingMode = child->isHorizontalWritingMode() != containerHasHorizontalWritingMode;
         if (hasOrthogonalWritingMode) {
             // FIXME: Properly support orthogonal writing mode.
             continue;
         }
 
         // 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());
 
     OwnPtr<GridSpan> positions = GridResolvedPosition::resolveGridPositionsFromStyle(*style(), child, direction);
     if (!positions) {
         offset = LayoutUnit();
         breadth = (direction == ForColumns) ? clientLogicalWidth() : clientLogicalHeight();
         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);
     GridResolvedPosition finalPosition = endIsAuto ? lastPosition : positions->resolvedFinalPosition;
 
     // Positioned children do not grow the grid, so we need to clamp the positions to avoid ending up outside of it.
     initialPosition = std::min<GridResolvedPosition>(initialPosition, lastPosition);
     finalPosition = std::min<GridResolvedPosition>(finalPosition, lastPosition);
 
     LayoutUnit start = startIsAuto ? LayoutUnit() : (direction == ForColumns) ?  m_columnPositions[initialPosition.toInt()] : m_rowPositions[initialPosition.toInt()];
     LayoutUnit end = endIsAuto ? (direction == ForColumns) ? logicalWidth() : logicalHeight() : (direction == ForColumns) ?  m_columnPositions[finalPosition.next().toInt()] : m_rowPositions[finalPosition.next().toInt()];
 
     breadth = end - start;
 
     if (startIsAuto)
         breadth -= (direction == ForColumns) ? borderStart() : borderBefore();
     else
         start -= ((direction == ForColumns) ? borderStart() : borderBefore());
 
     if (endIsAuto) {
         breadth -= (direction == ForColumns) ? borderEnd() : borderAfter();
         breadth -= scrollbarLogicalWidth();
     }
 
     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
 {
     ASSERT(m_gridItemCoordinate.contains(&gridItem));
     return m_gridItemCoordinate.get(&gridItem);
 }
 
 LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks) const
 {
@@ skipping 444 matching lines @@
 
     return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child));
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink