[CSS Grid Layout] Removing Content Alignment logic from track sizing alg

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 101 matching lines @@
     void ensureGrowthLimitIsBiggerThanBaseSize()
     {
         if (m_growthLimit != infinity && m_growthLimit < m_baseSize)
             m_growthLimit = m_baseSize;
     }
 
     LayoutUnit m_baseSize;
     LayoutUnit m_growthLimit;
 };
 
+struct ContentAlignmentData {
+    ContentAlignmentData(LayoutUnit position, LayoutUnit distribution)
+        : positionOffset(position)
+        , distributionOffset(distribution)
+    {
+    }
+
+    LayoutUnit positionOffset;
+    LayoutUnit distributionOffset;

[svillar, 2015/07/01 16:22:55]

missing "m_"

[jfernandez, 2015/07/02 10:04:22]

I've been told that when fields are accessible directly 'm_' prefix should be
avoided, which is the case of structs.  Anyway, let's add it since other strutcs
of in this class already do it.

[svillar, 2015/07/07 18:32:54]

The idea in general is not to allow public accessors.

[jfernandez, 2015/07/07 21:18:34]

Yeah, but when using structs it's all assumed as public, so fields are directly
accessed. GridSizingData structure follows the same approach.  

+};
+
 struct GridTrackForNormalization {
     GridTrackForNormalization(const GridTrack& track, double flex)
         : m_track(&track)
         , m_flex(flex)
         , m_normalizedFlexValue(track.baseSize() / flex)
     {
     }
 
     // Required by std::sort.
     GridTrackForNormalization& operator=(const GridTrackForNormalization& o)
@@ skipping 93 matching lines @@
     size_t m_childIndex;
 };
 
 struct LayoutGrid::GridSizingData {
     WTF_MAKE_NONCOPYABLE(GridSizingData);
     STACK_ALLOCATED();
 public:
     GridSizingData(size_t gridColumnCount, size_t gridRowCount)
         : columnTracks(gridColumnCount)
         , rowTracks(gridRowCount)
+        , columnTracksPositionsCacheIsValid(false)
+        , rowTracksPositionsCacheIsValid(false)
     {
     }
 
+    bool tracksPositionsCacheIsValid(GridTrackSizingDirection direction) const
+    {
+        return direction == ForColumns ? columnTracksPositionsCacheIsValid : rowTracksPositionsCacheIsValid;
+    }
+
+    void setTracksPositionsCacheValidity(GridTrackSizingDirection direction, bool isValid)
+    {
+        if (direction == ForColumns)
+            columnTracksPositionsCacheIsValid = isValid;
+        else
+            rowTracksPositionsCacheIsValid = isValid;
+    }
+
+    void setTracksPositionsCacheValidity(bool isValid)
+    {
+        columnTracksPositionsCacheIsValid = isValid;
+        rowTracksPositionsCacheIsValid = isValid;

[svillar, 2015/07/01 16:22:55]

Can be a single line. Also it's called just once so I don't think we need it (I
don't like the GridSizingData growing too much)

[jfernandez, 2015/07/02 10:04:21]

Well, we might be overprotecting our code. The reason for this is that we need
to use track positions to compute the grid item's area in certain cases. For
instance, whe item spans across several tracks and content alignment implies
adding an offset between those tracks. We were computing grid item's area based
on the base size of tracks it's laid of on, but this is not correct if there is
an offset between the positions of those tracks.

Hence we need to compute grid item's area based in the position of the tracks,
instead of their base size. It's can be also seen as a performance improvement,
since we don't have to iterate over the tracks the item is spanning across.  We
just need start and end tracks, and this works well too in the regular cases, no
alignment and no span.

The only concern, as you already mentioned, is that we might access tracks
position cache before (or inside) the track sizing algorithm has completely
defined base sized of all tracks, which obviously are used to determine their
positions. We still need the old way of getting grid item's area precisely
because of this, to be used inside the track sizing algorithm.

However, as I said before, we don't use this logic that much, and it's quite
simply, so perhaps we don't need such  control over the tracks positions cache. 

[svillar, 2015/07/02 11:19:28]

You didn't understand me, I was not talking about the cache but the need of that
method.

On 2015/07/02 10:04:21, jfernandez wrote:

[jfernandez, 2015/07/02 13:48:52]

Done.

+    }
+
     Vector<GridTrack> columnTracks;
     Vector<GridTrack> rowTracks;
     Vector<size_t> contentSizedTracksIndex;
 
     // Performance optimization: hold onto these Vectors until the end of Layout to avoid repeated malloc / free.
     Vector<GridTrack*> filteredTracks;
     Vector<GridItemWithSpan> itemsSortedByIncreasingSpan;
     Vector<GridTrack*> growBeyondGrowthLimitsTracks;
 
-    LayoutUnit rowsPositionOffset;
-    LayoutUnit rowsDistributionOffset;
-    LayoutUnit columnsPositionOffset;
-    LayoutUnit columnsDistributionOffset;
+    // Performance optimization: caching grid area size based on tracks positions.
+    bool columnTracksPositionsCacheIsValid;
+    bool rowTracksPositionsCacheIsValid;
 };
 
 struct GridItemsSpanGroupRange {
     Vector<GridItemWithSpan>::iterator rangeStart;
     Vector<GridItemWithSpan>::iterator rangeEnd;
 };
 
 LayoutGrid::LayoutGrid(Element* element)
     : LayoutBlock(element)
     , m_gridIsDirty(true)
@@ skipping 139 matching lines @@
 {
     return isFloatingOrOutOfFlowPositioned();
 }
 
 void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit& freeSpace)
 {
     const LayoutUnit initialFreeSpace = freeSpace;
     Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
     Vector<size_t> flexibleSizedTracksIndex;
     sizingData.contentSizedTracksIndex.shrink(0);
+    sizingData.setTracksPositionsCacheValidity(direction, false);
 
     // 1. Initialize per Grid track variables.
     for (size_t i = 0; i < tracks.size(); ++i) {
         GridTrack& track = tracks[i];
         GridTrackSize trackSize = gridTrackSize(direction, i);
         const GridLength& minTrackBreadth = trackSize.minTrackBreadth();
         const GridLength& maxTrackBreadth = trackSize.maxTrackBreadth();
 
         track.setBaseSize(computeUsedBreadthOfMinLength(direction, minTrackBreadth));
         track.setGrowthLimit(computeUsedBreadthOfMaxLength(direction, maxTrackBreadth, track.baseSize()));
@@ skipping 806 matching lines @@
     // the grid and its children are correctly laid out according to the new style rules.
     setNeedsLayout(LayoutInvalidationReason::GridChanged);
 
     m_grid.resize(0);
     m_gridItemCoordinate.clear();
     m_gridItemsOverflowingGridArea.resize(0);
     m_gridItemsIndexesMap.clear();
     m_gridIsDirty = true;
 }
 
+void LayoutGrid::applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit availableSpace)
+{
+    if (availableSpace <= 0
+        || (direction == ForColumns && styleRef().justifyContentDistribution() != ContentDistributionStretch)
+        || (direction == ForRows && styleRef().alignContentDistribution() != ContentDistributionStretch))
+        return;
+
+    // We consider auto-sized tracks as content-sized (min-content, max-content, auto).
+    Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
+    Vector<unsigned> autoSizedTracksIndex;
+    for (unsigned i = 0; i < tracks.size(); ++i) {
+        const GridTrackSize& trackSize = gridTrackSize(direction, i);
+        // If there is some flexible-sized track, they should have exhausted available space during sizing algorithm.
+        ASSERT(!trackSize.maxTrackBreadth().isFlex());

[svillar, 2015/07/07 18:32:54]

I don't get this ASSERT. If the max function is flex then it should still be
flex here, the functions do not change.

[jfernandez, 2015/07/07 21:18:34]

We shouldn't reach this point if there are flex tracks because they would
exhaust all free available space in the container.

+        if (trackSize.isContentSized())
+            autoSizedTracksIndex.append(i);

[svillar, 2015/07/01 16:22:55]

Sizing data already has the indexes of the content sized tracks. Why do you need
this? 

Ah perhaps it's because the new value of auto, I think we can use the current
vector of indexes, add here a FIXME, and leave it for a follow-up patch (adding
support for auto will likely force us to modify the current vector of indexes).

[jfernandez, 2015/07/02 10:04:22]

The reason is that SizingData structure uses the content-sized indexes vector as
a temporary data structure to run the track sizing logic. It's reset every time
its called to determine tracks sizes in a particular dimension (columns, rows).
We don't store that info, neither flex-sized track indexes, for both dimensions.


We can tweak the implementation calling the stretching logic right after
computing tracks sizes in the same dimension; we could also integrate the
content alignment logic inside the sizing algorithm, but at the end so it
doesn't interfere with the sizing logic. however, I think it's much better, and
spec compliant, to run any alignment code after all track sizing logic has been
already executed.

So, we need either to determine again the auto-sized tracks, for both
dimensions, or store them inside the GridSizing structure. You commented before
that you don' t like the idea of growing this data structure too much, so let's
think about it. 

+    }
+
+    unsigned numberOfAutoSizedTracks = autoSizedTracksIndex.size();
+    if (numberOfAutoSizedTracks < 1)
+        return;
+
+    // We are changing track sizes, so positions cache must be invalidated.
+    sizingData.setTracksPositionsCacheValidity(direction, false);
+
+    LayoutUnit sizeToIncrease = availableSpace / numberOfAutoSizedTracks;
+    for (const auto& trackIndex : autoSizedTracksIndex) {
+        GridTrack* track = tracks.data() + trackIndex;
+        // FIXME: Respecting the constraints imposed by max-height/max-width.

[svillar, 2015/07/01 16:22:55]

Not sure what you mean here. Which max-height/max-width?

[jfernandez, 2015/07/02 10:04:22]

Well, Alignment spec states that stretching should respect "constraints imposed
by max-height/max-width", which it's not so clear when we apply it to grid
tracks. Somehow, we must limit the final stretching breadth to these
constraints.  

I already asked to spec editors about this issue, so it's just a matter of wait
and interpret their answer.

[svillar, 2015/07/02 11:19:28]

The min and max in tracks are the min and max track sizing functions.

On 2015/07/02 10:04:22, jfernandez wrote:

[jfernandez, 2015/07/02 13:48:52]

Stretching can be applied only for 'auto-sized" alignment subjects, in this
case, grid tracks. We have assumed (to be confirmed by editors) that those
auto-sized are the ones using any content-sizing function: min-content,
max-content, auto, ... It's also possible to use minmax function, I guess using
content-sized limits; otherwise I guess it wouldn't fit in the "auto-sized"
category. 

However, if minmax function are used to define track sizes and at the same time
we use them as max-height/max-width restrictions, we never be able to stretch
tracks over its original size, as it's the purpose of this alignment value.

I might be missing something, or misunderstanding, but I think this point needs
further clarification.
  

+        LayoutUnit baseSize = track->baseSize() + sizeToIncrease;
+        track->setBaseSize(baseSize);
+    }
+}
+
 void LayoutGrid::layoutGridItems()
 {
     placeItemsOnGrid();
 
     LayoutUnit availableSpaceForColumns = availableLogicalWidth();
     LayoutUnit availableSpaceForRows = availableLogicalHeight(IncludeMarginBorderPadding);
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
     computeUsedBreadthOfGridTracks(ForColumns, sizingData, availableSpaceForColumns);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData.columnTracks));
     computeUsedBreadthOfGridTracks(ForRows, sizingData, availableSpaceForRows);
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForRows, sizingData.rowTracks));
 
-    computeContentPositionAndDistributionColumnOffset(availableSpaceForColumns, sizingData);
-    computeContentPositionAndDistributionRowOffset(availableSpaceForRows, sizingData);
+    applyStretchAlignmentToTracksIfNeeded(ForColumns, sizingData, availableSpaceForColumns);
+    applyStretchAlignmentToTracksIfNeeded(ForRows, sizingData, availableSpaceForRows);
 
-    populateGridPositions(sizingData);
+    populateGridPositions(sizingData, availableSpaceForColumns, availableSpaceForRows);
     m_gridItemsOverflowingGridArea.resize(0);
 
     for (LayoutBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
         if (child->isOutOfFlowPositioned()) {
             child->containingBlock()->insertPositionedObject(child);
             continue;
         }
 
         // Because the grid area cannot be styled, we don't need to adjust
         // the grid breadth to account for 'box-sizing'.
         LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child->hasOverrideContainingBlockLogicalWidth() ? child->overrideContainingBlockContentLogicalWidth() : LayoutUnit();
         LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
 
-        LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(*child, ForColumns, sizingData.columnTracks);
-        LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(*child, ForRows, sizingData.rowTracks);
+        LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(*child, ForColumns, sizingData);
+        LayoutUnit overrideContainingBlockContentLogicalHeight = gridAreaBreadthForChild(*child, ForRows, sizingData);
 
         SubtreeLayoutScope layoutScope(*child);
         if (oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth || (oldOverrideContainingBlockContentLogicalHeight != overrideContainingBlockContentLogicalHeight && child->hasRelativeLogicalHeight()))
             layoutScope.setNeedsLayout(child, LayoutInvalidationReason::GridChanged);
 
         child->setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
         child->setOverrideContainingBlockContentLogicalHeight(overrideContainingBlockContentLogicalHeight);
 
         // Stretching logic might force a child layout, so we need to run it before the layoutIfNeeded
         // call to avoid unnecessary relayouts. This might imply that child margins, needed to correctly
@@ skipping 110 matching lines @@
 
     offset = start;
 }
 
 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

[svillar, 2015/07/01 16:22:55]

Is this version still used after your patch?

[jfernandez, 2015/07/02 10:04:22]

Yes it is. As I commented before, we need something to use during the track
sizing algorithm, before tracks positions are not yet determined. 

 {
     const GridCoordinate& coordinate = cachedGridCoordinate(child);
     const GridSpan& span = (direction == ForColumns) ? coordinate.columns : coordinate.rows;
-    const Vector<LayoutUnit>& trackPositions = (direction == ForColumns) ? m_columnPositions : m_rowPositions;
-    if (span.resolvedFinalPosition.toInt() < trackPositions.size()) {
-        LayoutUnit startOftrack = trackPositions[span.resolvedInitialPosition.toInt()];
-        LayoutUnit endOfTrack = trackPositions[span.resolvedFinalPosition.toInt()];
-        return endOfTrack - startOftrack + tracks[span.resolvedFinalPosition.toInt()].baseSize();
-    }
     LayoutUnit gridAreaBreadth = 0;
     for (GridSpan::iterator trackPosition = span.begin(); trackPosition != span.end(); ++trackPosition)
         gridAreaBreadth += tracks[trackPosition.toInt()].baseSize();
-
     return gridAreaBreadth;
 }
 
-void LayoutGrid::populateGridPositions(const GridSizingData& sizingData)
+LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData) const
+{
+    ASSERT(sizingData.tracksPositionsCacheIsValid(direction));
+    // We need the cached value when available because Content Distribution alignment properties
+    // may have some influence in the final grid area breadth.
+    const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
+    const GridCoordinate& coordinate = cachedGridCoordinate(child);
+    const GridSpan& span = (direction == ForColumns) ? coordinate.columns : coordinate.rows;
+    const Vector<LayoutUnit>& trackPositions = (direction == ForColumns) ? m_columnPositions : m_rowPositions;
+    LayoutUnit startOftrack = trackPositions[span.resolvedInitialPosition.toInt()];
+    LayoutUnit endOfTrack = trackPositions[span.resolvedFinalPosition.toInt()];
+    return endOfTrack - startOftrack + tracks[span.resolvedFinalPosition.toInt()].baseSize();
+}
+
+void LayoutGrid::populateGridPositions(GridSizingData& sizingData, LayoutUnit availableSpaceForColumns, LayoutUnit availableSpaceForRows)
 {
     unsigned numberOfColumnTracks = sizingData.columnTracks.size();
     unsigned numberOfRowTracks = sizingData.rowTracks.size();
 
+    ContentAlignmentData columnOffset = computeContentPositionAndDistributionColumnOffset(availableSpaceForColumns, numberOfColumnTracks);
     m_columnPositions.resize(numberOfColumnTracks + 1);
-    m_columnPositions[0] = borderAndPaddingStart() + sizingData.columnsPositionOffset;
-    for (unsigned i = 0; i < numberOfColumnTracks; ++i)
-        m_columnPositions[i + 1] = m_columnPositions[i] + sizingData.columnsDistributionOffset + sizingData.columnTracks[i].baseSize();
+    m_columnPositions[0] = borderAndPaddingStart() + columnOffset.positionOffset;
+    for (unsigned i = 0; i < numberOfColumnTracks - 1; ++i)
+        m_columnPositions[i + 1] = m_columnPositions[i] + columnOffset.distributionOffset + sizingData.columnTracks[i].baseSize();
+    m_columnPositions[numberOfColumnTracks] = m_columnPositions[numberOfColumnTracks - 1] + sizingData.columnTracks[numberOfColumnTracks - 1].baseSize();

[svillar, 2015/07/01 16:22:55]

Why this change in the loop?

[jfernandez, 2015/07/02 10:04:22]

Well, since I had to change this function because of the new ContentAligmentData
structure w use now, I took the opportunity to fix a "bug", which while not
causing any functional issue it makes more complicated to determine grid items
position in case of RTL direction. This is also related to the change in the the
findChildLogicalPosition function. 

The issue is that track position cache store, actually, the position of the grid
lines defining those tracks. Without any distribution offset between tracks, end
line of track X is the same than start line of track X+1. Once we add an offset
this is not true (virtually) anymore, so I assumed what we store in tracks
positions cache is the position of the start line of each track, plus the end
line of the last one. It's precisely this last one line that we should not add
an additional distribution offset,m since the difference between two adjacent
grid lines must be just the track base size.

Adding that offset does not cause any issue with grid layout because we don't
use that last grid line in any computation, but when flipping positions for RTL
direction. I had to do some math trick to compensate that mistake, which now
isn't necessary.

[svillar, 2015/07/02 11:19:28]

I think this likely deserves a separate patch + test.

Also I think we aren't doing the right thing here. So if I am not wrong both
m_rowPositions and m_columnPositions store the positions of the track lines
without taking into account the alignment. That's a mistake and I think we
should fix that because those vectors are used to generate the computed style
for example which won't have the corrent values. Or am I wrong?

On 2015/07/02 10:04:22, jfernandez wrote:

[jfernandez, 2015/07/02 13:48:52]

I can do a new patch for this, not sure about the test because we are not
changing anything in terms of functionality. It just simplifies code dealing
with RTL in findChildLogicalPosition.
No, they indeed take into account alignment, of course. That's why we are adding
columnOffset/rowOffsets.  

That's a mistake and I think we
That's not the case; we are not using those vectors to generate computed values.
Computed values are just the base size of the tracks, not their positions. 

 
+    ContentAlignmentData rowOffset = computeContentPositionAndDistributionRowOffset(availableSpaceForRows, numberOfRowTracks);
     m_rowPositions.resize(numberOfRowTracks + 1);
-    m_rowPositions[0] = borderAndPaddingBefore() + sizingData.rowsPositionOffset;
-    for (unsigned i = 0; i < numberOfRowTracks; ++i)
-        m_rowPositions[i + 1] = m_rowPositions[i] + sizingData.rowsDistributionOffset + sizingData.rowTracks[i].baseSize();
+    m_rowPositions[0] = borderAndPaddingBefore() + rowOffset.positionOffset;
+    for (unsigned i = 0; i < numberOfRowTracks - 1; ++i)
+        m_rowPositions[i + 1] = m_rowPositions[i] + rowOffset.distributionOffset + sizingData.rowTracks[i].baseSize();
+    m_rowPositions[numberOfRowTracks] = m_rowPositions[numberOfRowTracks - 1] + sizingData.rowTracks[numberOfRowTracks - 1].baseSize();

[svillar, 2015/07/01 16:22:55]

Why this change in the loop?

[jfernandez, 2015/07/02 10:04:21]

Same as above comment.

+
+    sizingData.setTracksPositionsCacheValidity(true);
 }
 
 static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit trackBreadth, LayoutUnit childBreadth)
 {
     LayoutUnit offset = trackBreadth - childBreadth;
     switch (overflow) {
     case OverflowAlignmentSafe:
         // If overflow is 'safe', we have to make sure we don't overflow the 'start'
         // edge (potentially cause some data loss as the overflow is unreachable).
         return std::max<LayoutUnit>(0, offset);
@@ skipping 267 matching lines @@
 {
     return isLeftToRight ? LayoutUnit() : availableSpace;
 }
 
 static inline LayoutUnit offsetToEndEdge(bool isLeftToRight, LayoutUnit availableSpace)
 {
     return !isLeftToRight ? LayoutUnit() : availableSpace;
 }
 
 
-static bool contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfGridTracks, LayoutUnit& positionOffset, LayoutUnit& distributionOffset)
+static PassOwnPtr<ContentAlignmentData> contentDistributionOffset(LayoutUnit availableFreeSpace, ContentPosition& fallbackPosition, ContentDistributionType distribution, unsigned numberOfGridTracks)
 {
     if (distribution != ContentDistributionDefault && fallbackPosition == ContentPositionAuto)
         fallbackPosition = resolveContentDistributionFallback(distribution);
 
     if (availableFreeSpace <= 0)
-        return false;
+        return nullptr;
 
+    LayoutUnit distributionOffset;
     switch (distribution) {
     case ContentDistributionSpaceBetween:
         if (numberOfGridTracks < 2)
-            return false;
-        distributionOffset = availableFreeSpace / (numberOfGridTracks - 1);
-        positionOffset = 0;
-        return true;
+            return nullptr;
+        return adoptPtr(new ContentAlignmentData(0, availableFreeSpace / (numberOfGridTracks - 1)));
     case ContentDistributionSpaceAround:
         if (numberOfGridTracks < 1)
-            return false;
+            return nullptr;
         distributionOffset = availableFreeSpace / numberOfGridTracks;
-        positionOffset = distributionOffset / 2;
-        return true;
+        return adoptPtr(new ContentAlignmentData(distributionOffset / 2, distributionOffset));
     case ContentDistributionSpaceEvenly:
         distributionOffset = availableFreeSpace / (numberOfGridTracks + 1);
-        positionOffset = distributionOffset;
-        return true;
+        return adoptPtr(new ContentAlignmentData(distributionOffset, distributionOffset));
     case ContentDistributionStretch:
-        distributionOffset = 0;
-        positionOffset = 0;
-        return true;
+        return adoptPtr(new ContentAlignmentData(0, 0));
     case ContentDistributionDefault:
-        distributionOffset = 0;
-        positionOffset = 0;
-        return false;
+        return nullptr;
     }
 
     ASSERT_NOT_REACHED();
-    return false;
+    return nullptr;
 }

[svillar, 2015/07/01 16:22:55]

Why are you allocating all those objects in the stack? 

I think I prefer you to pass a reference to a ContentAlingment object as
parametter and return a boolean as you've been doing.

[jfernandez, 2015/07/02 10:04:22]

Yes, that was my first idea. The issue is that I need to define an invalid state
for the ContentAlignmentData structure. In my first implementation I defined
such invalid state as -1 on both offset values. Some WebKit reviewer told me
that using a magical number for defining an invalid state was not a good idea,
so I thought nullptr could express that state much better. I'd be happy to get
rid of the pointer allocation, but for that we should agree on a better way to
define the invalid state.

[svillar, 2015/07/02 11:19:28]

You mean is not enough with returning false but you have to also store an
invalid state somewhere else?

[jfernandez, 2015/07/02 13:48:52]

Well, the function returns now data structure, instead of using inout arguments.
I think is clearer  and more flexible to use the data structure. What I mean, if
we agree don this, is that I need such data structure to be validated somehow, I
guess based on the data it holds.

 
-void LayoutGrid::computeContentPositionAndDistributionColumnOffset(LayoutUnit availableFreeSpace, GridSizingData& sizingData) const
+ContentAlignmentData LayoutGrid::computeContentPositionAndDistributionColumnOffset(LayoutUnit availableFreeSpace, unsigned numberOfGridTracks) const
 {
     ContentPosition position = styleRef().justifyContentPosition();
     ContentDistributionType distribution = styleRef().justifyContentDistribution();
     // If <content-distribution> value can't be applied, 'position' will become the associated
     // <content-position> fallback value.
-    if (contentDistributionOffset(availableFreeSpace, position, distribution, sizingData.columnTracks.size(), sizingData.columnsPositionOffset, sizingData.columnsDistributionOffset))
-        return;
+    OwnPtr<ContentAlignmentData> contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);
+    if (contentAlignment)
+        return *contentAlignment;
 
-    OverflowAlignment overflow = styleRef().justifyContentOverflowAlignment();
-    if (overflow == OverflowAlignmentSafe && availableFreeSpace <= 0)
-        return;
+    if (availableFreeSpace <= 0 && styleRef().justifyContentOverflowAlignment() == OverflowAlignmentSafe)
+        return {0, 0};
 
     switch (position) {
     case ContentPositionLeft:
-        sizingData.columnsPositionOffset = 0;
-        return;
+        return {0, 0};
     case ContentPositionRight:
-        sizingData.columnsPositionOffset = availableFreeSpace;
-        return;
+        return {availableFreeSpace, 0};
     case ContentPositionCenter:
-        sizingData.columnsPositionOffset = availableFreeSpace / 2;
-        return;
+        return {availableFreeSpace / 2, 0};
     case ContentPositionFlexEnd:
         // Only used in flex layout, for other layout, it's equivalent to 'end'.
     case ContentPositionEnd:
-        sizingData.columnsPositionOffset = offsetToEndEdge(style()->isLeftToRightDirection(), availableFreeSpace);
-        return;
+        return {offsetToEndEdge(styleRef().isLeftToRightDirection(), availableFreeSpace), 0};
     case ContentPositionFlexStart:
         // Only used in flex layout, for other layout, it's equivalent to 'start'.
     case ContentPositionStart:
-        sizingData.columnsPositionOffset = offsetToStartEdge(style()->isLeftToRightDirection(), availableFreeSpace);
-        return;
+        return {offsetToStartEdge(styleRef().isLeftToRightDirection(), availableFreeSpace), 0};
     case ContentPositionBaseline:
     case ContentPositionLastBaseline:
         // FIXME: These two require implementing Baseline Alignment. For now, we always 'start' align the child.
         // crbug.com/234191
-        sizingData.columnsPositionOffset = offsetToStartEdge(style()->isLeftToRightDirection(), availableFreeSpace);
-        return;
+        return {offsetToStartEdge(styleRef().isLeftToRightDirection(), availableFreeSpace), 0};
     case ContentPositionAuto:
         break;
     }
 
     ASSERT_NOT_REACHED();
+    return {0, 0};
 }
 
-void LayoutGrid::computeContentPositionAndDistributionRowOffset(LayoutUnit availableFreeSpace, GridSizingData& sizingData) const
+ContentAlignmentData LayoutGrid::computeContentPositionAndDistributionRowOffset(LayoutUnit availableFreeSpace, unsigned numberOfGridTracks) const
 {
     ContentPosition position = styleRef().alignContentPosition();
     ContentDistributionType distribution = styleRef().alignContentDistribution();
     // If <content-distribution> value can't be applied, 'position' will become the associated
     // <content-position> fallback value.
-    if (contentDistributionOffset(availableFreeSpace, position, distribution, sizingData.rowTracks.size(), sizingData.rowsPositionOffset, sizingData.rowsDistributionOffset))
-        return;
+    OwnPtr<ContentAlignmentData> contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks);
+    if (contentAlignment)
+        return *contentAlignment;
 
-    OverflowAlignment overflow = styleRef().alignContentOverflowAlignment();
-    if (overflow == OverflowAlignmentSafe && availableFreeSpace <= 0)
-        return;
+    if (availableFreeSpace <= 0 && styleRef().alignContentOverflowAlignment() == OverflowAlignmentSafe)
+        return {0, 0};
 
     switch (position) {
     case ContentPositionLeft:
         // The align-content's axis is always orthogonal to the inline-axis.
-        sizingData.rowsPositionOffset = 0;
-        return;
+        return {0, 0};
     case ContentPositionRight:
         // The align-content's axis is always orthogonal to the inline-axis.
-        sizingData.rowsPositionOffset = 0;
-        return;
+        return {0, 0};
     case ContentPositionCenter:
-        sizingData.rowsPositionOffset = availableFreeSpace / 2;
-        return;
+        return {availableFreeSpace / 2, 0};
     case ContentPositionFlexEnd:
         // Only used in flex layout, for other layout, it's equivalent to 'End'.
     case ContentPositionEnd:
-        sizingData.rowsPositionOffset = availableFreeSpace;
-        return;
+        return {availableFreeSpace, 0};
     case ContentPositionFlexStart:
         // Only used in flex layout, for other layout, it's equivalent to 'Start'.
     case ContentPositionStart:
-        sizingData.rowsPositionOffset = 0;
-        return;
+        return {0, 0};
     case ContentPositionBaseline:
     case ContentPositionLastBaseline:
         // FIXME: These two require implementing Baseline Alignment. For now, we always 'start' align the child.
         // crbug.com/234191
-        sizingData.rowsPositionOffset = 0;
-        return;
+        return {0, 0};
     case ContentPositionAuto:
         break;
     }
 
     ASSERT_NOT_REACHED();
+    return {0, 0};
 }
 
 LayoutPoint LayoutGrid::findChildLogicalPosition(const LayoutBox& child, GridSizingData& sizingData) const
 {
     LayoutUnit columnPosition = columnPositionForChild(child);
     // We stored m_columnPositions's data ignoring the direction, hence we might need now
     // to translate positions from RTL to LTR, as it's more convenient for painting.
-    if (!style()->isLeftToRightDirection())
-        columnPosition = (m_columnPositions[m_columnPositions.size() - 1] + borderAndPaddingLogicalLeft() + sizingData.columnsPositionOffset) - columnPosition - sizingData.columnsDistributionOffset - child.logicalWidth();
+    if (!style()->isLeftToRightDirection()) {
+        LayoutUnit rightEdgePosition = m_columnPositions[m_columnPositions.size() - 1] + borderAndPaddingLogicalLeft();
+        LayoutUnit leftEdgePosition =  m_columnPositions[0] - borderAndPaddingStart();
+        columnPosition = rightEdgePosition + leftEdgePosition - columnPosition - child.logicalWidth();
+    }

[svillar, 2015/07/01 16:22:55]

Not sure that I understand this, perhaps it's because of the names 

rightEdgePosition -> gridRightEdgePosition
leftEdgePosition -> gridLeftEdgePosition
columnPosition -> childColumnPosition

or something similar that could help reading the code.

[jfernandez, 2015/07/02 10:04:22]

Are the names changes enough to make the code understandable ? 

The idea behind this logic is to flip column track positions so first column
track will have the highest value, and viceversa, when grid container uses RTL
direction. Using logical values, as it inline-flow rightest edge would start at
0px, makes our grid computation easier, but for perhaps, we need to adapt
locations to become absolute positions, which will be then interpreted by
painting logic according to flow direction.

 
     return LayoutPoint(columnPosition, rowPositionForChild(child));
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink