[CSS Grid Layout] Support span in auto-placement algorithm

[CSS Grid Layout] Support span in auto-placement algorithm

Modify auto-placement algorithm in order to support span in both
definite and automatic positions.

This patch fixes examples like:
  grid-row: auto;
  grid-column: 2 / span 3;

And also:
  grid-row: auto;
  grid-column: span 3;

BUG=353789
TEST=fast/css-grid-layout/grid-item-auto-placement-definite-span.html
TEST=fast/css-grid-layout/grid-item-auto-placement-automatic-span.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175343

[Julien - ping for review, 2014-03-20 18:33:14]

https://codereview.chromium.org/196943026/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html
(right):

https://codereview.chromium.org/196943026/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:5:
<style type="text/css">
No need for |type|.

https://codereview.chromium.org/196943026/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:51:
<div class="grid gridAutoFlowRow">
Shouldn't you define the grid's columns and rows if you want your grid items to
have any dimension (especially for 'width' which is auto and should default to 0
here)?

This test could be a check-layout.js test (if you set up the rows / columns
accordingly it's easy to know where the grid items fell) but I am not going to
push hard for it to be.

https://codereview.chromium.org/196943026/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/196943026/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:144: PassOwnPtr<GridCoordinate>
nextEmptyGridArea(size_t finalTrackIndex = 0)
Not a huge fan of:
a) the implicit parameter as not passing the parameter if you have a defined
span (that is not 1) is just wrong.
b) passing a magical position when what you actually want is to account for the
span.

For me, b) obscures the intent of the code by not being clear that what we care
about is the grid item spanning several grid areas.

[Manuel Rego, 2014-03-20 23:18:56]

Thanks for the review, I've just uploaded a new version of the patch.

On 2014/03/20 18:33:14, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/196943026/diff/1/LayoutTests/fast/css-grid-la...
>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:5:
> <style type="text/css">
> No need for |type|.

Fixed.

>
https://codereview.chromium.org/196943026/diff/1/LayoutTests/fast/css-grid-la...
>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:51:
> <div class="grid gridAutoFlowRow">
> Shouldn't you define the grid's columns and rows if you want your grid items
to
> have any dimension (especially for 'width' which is auto and should default to
0
> here)?

I've used grid-auto-rows and grid-auto-columns now.

> This test could be a check-layout.js test (if you set up the rows / columns
> accordingly it's easy to know where the grid items fell) but I am not going to
> push hard for it to be.

Not a big deal, so I've changed it to be a check-layout test.

>
https://codereview.chromium.org/196943026/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderGrid.cpp:144: PassOwnPtr<GridCoordinate>
> nextEmptyGridArea(size_t finalTrackIndex = 0)
> Not a huge fan of:
> a) the implicit parameter as not passing the parameter if you have a defined
> span (that is not 1) is just wrong.
> b) passing a magical position when what you actually want is to account for
the
> span.
> 
> For me, b) obscures the intent of the code by not being clear that what we
care
> about is the grid item spanning several grid areas.

I've removed the implicit parameter and tried to improve it. Let me know what do
you think now.

[Julien - ping for review, 2014-03-21 18:18:02]

More comments, the patch looks much nicer, thanks!

https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:125: bool areCellsEmpty(size_t
spanningAreas)
Note that this introduces some O(N^2) behavior. It's not a big deal as we expect
spanning areas to be small but it may be worth a comment if we find this to be
an issue (there are definitely better data structure that could alleviate this).

Also would this work with spanning items as we don't insert them repeatedly into
m_grid? (what I mean is that grid-row: 1 / span 2; grid-column: 1; will be
inserted into m_grid[0][0] but not m_grid[1][0])

https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:144: PassOwnPtr<GridCoordinate>
nextEmptyGridArea(size_t spanningAreas)
It is weird to have the *same* |spanningAreas| for both directions when we know
they can be different. I would be way more comfortable with passing both even if
you only use one for now.

https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:155: OwnPtr<GridCoordinate> result =
adoptPtr(new GridCoordinate(rowSpan, columnSpan));
I wonder if just having a single m_direction == ForColumns check would simplify
this code (it's a nit though).

https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:843: if (OwnPtr<GridCoordinate>
emptyGridArea =
iterator.nextEmptyGridArea(majorAxisPositions->finalPositionIndex -
majorAxisPositions->initialPositionIndex + 1)) {
How about adding a helper function to return the span from a GridSpan?

[Manuel Rego, 2014-03-24 12:33:21]

Thanks for the review.

On 2014/03/21 18:18:02, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:125: bool areCellsEmpty(size_t
> spanningAreas)
> Note that this introduces some O(N^2) behavior. It's not a big deal as we
expect
> spanning areas to be small but it may be worth a comment if we find this to be
> an issue (there are definitely better data structure that could alleviate
this).

Added comment.

> Also would this work with spanning items as we don't insert them repeatedly
into
> m_grid? (what I mean is that grid-row: 1 / span 2; grid-column: 1; will be
> inserted into m_grid[0][0] but not m_grid[1][0])

We're currently inserting items repeatedly. So with current code (and with this
patch too) your example:
    grid-row: 1 / span 2; grid-column: 1;
will be inserted into m_grid[0][0] and m_grid[1][0].
Otherwise this patch won't work.

>
https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:144: PassOwnPtr<GridCoordinate>
> nextEmptyGridArea(size_t spanningAreas)
> It is weird to have the *same* |spanningAreas| for both directions when we
know
> they can be different. I would be way more comfortable with passing both even
if
> you only use one for now.

This is part of the GridIterator that has already a direction, so I think it is
ok
to keep it just for one direction.
If in the future we need to have both directions we can always modify it, but
I think it's better avoid to add an extra param now.

>
https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:155: OwnPtr<GridCoordinate> result =
> adoptPtr(new GridCoordinate(rowSpan, columnSpan));
> I wonder if just having a single m_direction == ForColumns check would
simplify
> this code (it's a nit though).

I've changed it, I agree that it might be better. Together with the only if in
areCellsEmpty() it makes it clearer.

>
https://codereview.chromium.org/196943026/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:843: if (OwnPtr<GridCoordinate>
> emptyGridArea =
> iterator.nextEmptyGridArea(majorAxisPositions->finalPositionIndex -
> majorAxisPositions->initialPositionIndex + 1)) {
> How about adding a helper function to return the span from a GridSpan?

Good idea, I've added a method for this.

[Julien - ping for review, 2014-04-04 00:10:16]

Sorry for the long delay in the review.

https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:145: PassOwnPtr<GridCoordinate>
nextEmptyGridArea(size_t spanningPositions)
|spanningPositions| confuses me: that's because it's using a plural which I
thought meant that we handled 2 directions. How about renaming it to
positionSpanInDirectionOfIteration (or something equivalent but less verbose).

This previous confusion comes from the fact that to fully support the grid
specification we will need to account for the spans in *both* direction. That's
because you need to find a position that does not overlap any "occupied grid
cell".

I would be much more comfortable passing the 2 spans as part of this patch (to
iron out the API) even if you ignore about the one in the cross direction.

https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/GridCoordinate.h (right):

https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/GridCoordinate.h:117: size_t spanningPositions()
const
This name is also confusing for me. I would rather to name it integerSpan (or
just "span" even if that would be ambiguous).

[Manuel Rego, 2014-04-07 10:54:31]

Thanks for the review.

On 2014/04/04 00:10:16, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:145: PassOwnPtr<GridCoordinate>
> nextEmptyGridArea(size_t spanningPositions)
> |spanningPositions| confuses me: that's because it's using a plural which I
> thought meant that we handled 2 directions. How about renaming it to
> positionSpanInDirectionOfIteration (or something equivalent but less verbose).
> 
> This previous confusion comes from the fact that to fully support the grid
> specification we will need to account for the spans in *both* direction.
That's
> because you need to find a position that does not overlap any "occupied grid
> cell".
> 
> I would be much more comfortable passing the 2 spans as part of this patch (to
> iron out the API) even if you ignore about the one in the cross direction.

At the end, I've changed my mind and I'm now passing 2 arguments as you
suggested. I think I'll use it in order to support "auto / span 3" and similar
things in a later patch.

>
https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/st...
> Source/core/rendering/style/GridCoordinate.h:117: size_t spanningPositions()
> const
> This name is also confusing for me. I would rather to name it integerSpan (or
> just "span" even if that would be ambiguous).

Fixed.

[Manuel Rego, 2014-04-07 22:26:27]

On 2014/04/07 10:54:31, Manuel Rego wrote:
> On 2014/04/04 00:10:16, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/196943026/diff/60001/Source/core/rendering/Re...
> > Source/core/rendering/RenderGrid.cpp:145: PassOwnPtr<GridCoordinate>
> > nextEmptyGridArea(size_t spanningPositions)
> > |spanningPositions| confuses me: that's because it's using a plural which I
> > thought meant that we handled 2 directions. How about renaming it to
> > positionSpanInDirectionOfIteration (or something equivalent but less
verbose).
> > 
> > This previous confusion comes from the fact that to fully support the grid
> > specification we will need to account for the spans in *both* direction.
> That's
> > because you need to find a position that does not overlap any "occupied grid
> > cell".
> > 
> > I would be much more comfortable passing the 2 spans as part of this patch
(to
> > iron out the API) even if you ignore about the one in the cross direction.
> 
> At the end, I've changed my mind and I'm now passing 2 arguments as you
> suggested. I think I'll use it in order to support "auto / span 3" and similar
> things in a later patch.

Finally I'm coming back to just 1 argument in nextEmptyGridArea().
Let's try to exaplain the reason (I have realized when trying to rebase
the second patch to fix "auto / span 3" over the new version of this
patch).

Basically in GridIterator we set a |fixedTrackIndex| for the |direction|
we pass in the constructor.
Then we vary the other direction index looking for empty grid areas.

As we set the |fixedTrackIndex| from the outside, we can know the span
value for it and pass it to nextEmpyGridArea() (as I'm doing in the patch).
However, we cannot know the span for the other direction index, as it
might depend on the initial position, for example if we have "span C", we
would need to re-calculate it as we advance the varying index. In order
to do that we would need more information from the grid inside
RenderGrid than what we have now.

So, we'll get empty grid areas within only 1 row/column in the varying
direction, and then outside GridIterator we will check if the item fits
or not there. Otherwise, we'll request a new empty grid area.

Maybe I'm missing something, and we might end up needing 2
arguments in nextEmptyGridArea(), but I think it's better to add
them when we are really sure than now.

[Manuel Rego, 2014-04-09 11:23:17]

I've been doing more progress on this and finally uploaded a new patch
fixing both definite and automatic span.

Sorry for re-doing it completely but I think current patch makes things
easier to understand.

[Manuel Rego, 2014-04-15 11:07:28]

Uploaded rebased version of the patch after r171156 (where GridResolvedPosition
was introduced).

[Julien - ping for review, 2014-05-16 10:08:09]

https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html
(right):

https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:12:
grid-column: span 2;
That should resolve to 'auto / auto' when we expand the shorthand. Is that
intended?

https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html
(right):

https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:47:
}
Based on our discussions, we will need more tests as we don't cover:
- explicit grid lines.
- spanned with grid lines.

https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:803: } else if
(autoPlacementMinorAxisDirection() == ForRows) {
It seems to me that we need to make the initial grid as tall as the tallest
known span, regardless of the auto-placement direction.

We don't strictly have to do this resizing and could totally resize the grid as
needed, which I think would simplify the code as we *have* to handle growing the
grid several rows / columns anyway.

https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:841: PassOwnPtr<GridCoordinate>
RenderGrid::implicitEmptyGridAreaSpecifiedPositions(const RenderBox* gridItem,
GridTrackSizingDirection specifiedDirection, const GridSpan& specifiedPositions)
const
What is an implicit empty grid area?

There are 2 patterns when trying to find an empty grid area:
- either we have a fixed axis, in which case we must find a position, if needed
past the end of the grid.
- try placing the grid item, in which case we just return NULL (no position) at
the end.

I think we should try to make this code more readable by making the 2 patterns
more obvious. For example, you could have a emptyGridAreaAtSpecifiedPosition()
(that can't return NULL) vs tryFindingEmptyGridAreaAtSpecifiedPosition(). The
names can be improved.

https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:857: OwnPtr<GridSpan>
crossDirectionPositions =
GridResolvedPosition::resolveGridPositionsFromAutoPlacementPosition(*style(),
*gridItem, crossDirection, crossDirectionInitialPositionIndex);
I think we should explain why we need to resolve the position over and over.
This is because span 2 'c' can have a different resolution based on your
position.

Also only some cases would have a varying span value (explicit grid line and
integer don't need that).

https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:868: void
RenderGrid::insertItemIntoGridGrowingIfNeeded(RenderBox* gridItem, const
GridCoordinate& coordinate, GridTrackSizingDirection growingDirection)
I don't like this split for several reasons:
* insertItemIntoGrid() has only one caller - this function - which means that we
don't really need the difference.
* fundamentally, growing the grid is an implementation detail that shouldn't be
exposed to the caller (who shouldn't care really).
* Also we don't want to allow callers to call insertItemIntoGrid as you could
potentially write past outside the grid (using a defense-in-depth pattern is
great).

[Manuel Rego, 2014-05-23 11:31:54]

Thanks for the review. I've just uploaded a new version of the patch.

On 2014/05/16 10:08:09, Julien Chaffraix - CET wrote:
>
https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
> File
> LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html
> (right):
> 
>
https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:12:
> grid-column: span 2;
> That should resolve to 'auto / auto' when we expand the shorthand. Is that
> intended?

I'm not sure, I think it resolves to "span 2 / auto". From the spec:
http://dev.w3.org/csswg/css-grid/#placement-shorthands
"When the second value is omitted, if the first value is a <custom-ident>,
the grid-row-end/grid-column-end longhand is also set to that <custom-ident>;
otherwise, it is set to auto."

On top of that, in the spec there're some examples like that:
EXAMPLE 21 or EXAMPLE 32.

My intention was to define an element that takes 2 columns. And current
implementation works like that.

>
https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
> File
> LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html
> (right):
> 
>
https://codereview.chromium.org/196943026/diff/140001/LayoutTests/fast/css-gr...
>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:47:
> }
> Based on our discussions, we will need more tests as we don't cover:
> - explicit grid lines.

I'm not sure we need new tests for named grid lines, I don't think they cover
anything extra that current tests with integers. Maybe I'm missing something.
Of course, I can add them if needed, it's not a big deal.

> - spanned with grid lines.

I've added cases for the span using named grid lines (that varies from taking
1, 2 or 3 columns/rows).

>
https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
> File Source/core/rendering/RenderGrid.cpp (right):
> 
>
https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:803: } else if
> (autoPlacementMinorAxisDirection() == ForRows) {
> It seems to me that we need to make the initial grid as tall as the tallest
> known span, regardless of the auto-placement direction.

We could do it, but it's only needed for the minor axis direction, as you can
see in the step 3 of the auto-placement algorithm in the spec:
http://dev.w3.org/csswg/css-grid/#auto-placement-algo

So, I think it's good to avoid resolving positions when it's not mandatory.

> We don't strictly have to do this resizing and could totally resize the grid
as
> needed, which I think would simplify the code as we *have* to handle growing
the
> grid several rows / columns anyway.

Let me put an example, when it would be needed to resize here:
* We have defined a 2x2 grid explicit grid with 4 items with definite positions,
  each of them in one cell. In addition we have 2 extra auto-positioned items,
  one taking just one 1 cell and the another one taking an area of 3x3
  (9 cells).
* grid-auto-flow property is row, so the grid grows in the row direction.
* We place the not auto-positioned items. So we have the 2x2 grid full.
* Once we're going to place the auto-positioned item that takes 1 cell, if we
  haven't grow the grid before, we would be placing it in row 3, column 1.
  Actually according to the algorithm it should be placed in row 1, column 3.
  So, we need to grow the grid here, to be sure that we're placing the item in
  the rigth position.

>
https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:841: PassOwnPtr<GridCoordinate>
> RenderGrid::implicitEmptyGridAreaSpecifiedPositions(const RenderBox* gridItem,
> GridTrackSizingDirection specifiedDirection, const GridSpan&
specifiedPositions)
> const
> What is an implicit empty grid area?

The name is wrong, but I was thinking in the explicit vs implicit grid.
Where implicit empty grid area will be an area in the implicit grid, but
actually it's an area outside of current grid.

> There are 2 patterns when trying to find an empty grid area:
> - either we have a fixed axis, in which case we must find a position, if
needed
> past the end of the grid.
> - try placing the grid item, in which case we just return NULL (no position)
at
> the end.
> 
> I think we should try to make this code more readable by making the 2 patterns
> more obvious. For example, you could have a emptyGridAreaAtSpecifiedPosition()
> (that can't return NULL) vs tryFindingEmptyGridAreaAtSpecifiedPosition(). The
> names can be improved.

Yeah, I agree that we need to find better names. I've used the following ones
in the new patch:
* createEmptyGridAreaAtSpecifiedPositionsOutsideGrid() cannot return NULL.
* findEmptyGridAreaAtSpecifiedPositionsInsideGrid() can return NULL.

>
https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:857: OwnPtr<GridSpan>
> crossDirectionPositions =
> GridResolvedPosition::resolveGridPositionsFromAutoPlacementPosition(*style(),
> *gridItem, crossDirection, crossDirectionInitialPositionIndex);
> I think we should explain why we need to resolve the position over and over.
> This is because span 2 'c' can have a different resolution based on your
> position.
>
> Also only some cases would have a varying span value (explicit grid line and
> integer don't need that).

I've added a comment trying to explain this.

>
https://codereview.chromium.org/196943026/diff/140001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:868: void
> RenderGrid::insertItemIntoGridGrowingIfNeeded(RenderBox* gridItem, const
> GridCoordinate& coordinate, GridTrackSizingDirection growingDirection)
> I don't like this split for several reasons:
> * insertItemIntoGrid() has only one caller - this function - which means that
we
> don't really need the difference.

BTW, insertItemIntoGrid() is called from 2 places more: addChild() and
placeItemsOnGrid().

> * fundamentally, growing the grid is an implementation detail that shouldn't
be
> exposed to the caller (who shouldn't care really).
> * Also we don't want to allow callers to call insertItemIntoGrid as you could
> potentially write past outside the grid (using a defense-in-depth pattern is
> great).

Anyway, I agree with you and also think that it's better to move the growing
inside insertItemIntoGrid() (actually addChild() was already growing the grid
if needed before calling it).

So, I've removed the new insertItemIntoGridGrowingIfNeeded().

[eseidel, 2014-05-28 23:29:35]

jchaffraix: ptal?

[Julien - ping for review, 2014-05-29 21:06:59]

lgtm with comments.

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:752: growGrid(ForColumns,
coordinate.columns.resolvedFinalPosition.toInt());
I think we should just unconditionally ensure enough grid rows / columns and
have an API to grow both directions at once. That can be done in a follow-up
change.

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:838: } else if
(autoPlacementMinorAxisDirection() == ForColumns) {
I still don't agree with being conditional here.

If you have a 3x3 items, your grid should be *at least* 3x3 regardless of where
we place the item. Your example does show that it can be bigger, which is
perfectly fine. Checking for the auto-placement axis is artificial, shouldn't
change the final grid size and prevent us from pre-allocating the grid to a
minimum sensible size.

(btw, please comment using the UI instead of replying as it breaks rietveld so I
can't easily reply to your comments)

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/s...
File Source/core/rendering/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/s...
Source/core/rendering/style/GridResolvedPosition.cpp:23: void
GridResolvedPosition::initialAndFinalPositionsFromStyle(const RenderStyle&
gridContainerStyle, const RenderBox& gridItem, GridTrackSizingDirection
direction, GridPosition& initialPosition, const GridPositionSide
initialPositionSide, GridPosition& finalPosition, const GridPositionSide
finalPositionSide)
I don't see what this refactoring gives us:
- the function now takes 7 arguments (!!!!)
- callers have to compute the GridPositionSide (aka code duplication)
- callers have to assemble a GridSpan anyway.j

You also can access the members of the GridSpan easily so it seems to me that we
don't get much from that.

https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/s...
Source/core/rendering/style/GridResolvedPosition.cpp:40: PassOwnPtr<GridSpan>
GridResolvedPosition::resolveGridPositionsFromAutoPlacementPosition(const
RenderStyle& gridContainerStyle, const RenderBox& gridItem,
GridTrackSizingDirection direction, const GridResolvedPosition&
resolvedInitialPosition)
This function cannot return nullptr so we should make it return a GridSpan.

[Manuel Rego, 2014-06-02 20:19:12]

On 2014/05/29 21:06:59, Julien Chaffraix - PST wrote:
> lgtm with comments.

Thanks for the review!

>
https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:752: growGrid(ForColumns,
> coordinate.columns.resolvedFinalPosition.toInt());
> I think we should just unconditionally ensure enough grid rows / columns and
> have an API to grow both directions at once. That can be done in a follow-up
> change.

I'll do this in a later patch.

>
https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:838: } else if
> (autoPlacementMinorAxisDirection() == ForColumns) {
> I still don't agree with being conditional here.
> 
> If you have a 3x3 items, your grid should be *at least* 3x3 regardless of
where
> we place the item. Your example does show that it can be bigger, which is
> perfectly fine. Checking for the auto-placement axis is artificial, shouldn't
> change the final grid size and prevent us from pre-allocating the grid to a
> minimum sensible size.

I removing the if here, as it's not needed.

What is needed is to grow the grid here and not while processing
the elements (as I explained in the previous comment).

> (btw, please comment using the UI instead of replying as it breaks rietveld so
I
> can't easily reply to your comments)

Yeah, sorry for messing it up in the previous message.

>
https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/s...
> Source/core/rendering/style/GridResolvedPosition.cpp:23: void
> GridResolvedPosition::initialAndFinalPositionsFromStyle(const RenderStyle&
> gridContainerStyle, const RenderBox& gridItem, GridTrackSizingDirection
> direction, GridPosition& initialPosition, const GridPositionSide
> initialPositionSide, GridPosition& finalPosition, const GridPositionSide
> finalPositionSide)
> I don't see what this refactoring gives us:
> - the function now takes 7 arguments (!!!!)
> - callers have to compute the GridPositionSide (aka code duplication)
> - callers have to assemble a GridSpan anyway.j
> 
> You also can access the members of the GridSpan easily so it seems to me that
we
> don't get much from that.

Basically the ifs inside initialAndFinalPositionsFromStyle() would be repeated
in both resolveGridPositionsFromAutoPlacementPosition() and
resolveGridPositionsFromStyle().

A GridSpan has GridResolvedPositions, no GridPositions, so I cannot use it
directly here.

Anyway, I've moved the GridPositionSide calculation to an inline function,
as it's pretty simple and that makes the method to take 5 arguments instead
of 7.

>
https://codereview.chromium.org/196943026/diff/160001/Source/core/rendering/s...
> Source/core/rendering/style/GridResolvedPosition.cpp:40: PassOwnPtr<GridSpan>
> GridResolvedPosition::resolveGridPositionsFromAutoPlacementPosition(const
> RenderStyle& gridContainerStyle, const RenderBox& gridItem,
> GridTrackSizingDirection direction, const GridResolvedPosition&
> resolvedInitialPosition)
> This function cannot return nullptr so we should make it return a GridSpan.

True, I've modified it.

[Manuel Rego, 2014-06-02 20:24:08]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-02 20:24:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rego@igalia.com/196943026/180001

[commit-bot: I haz the power, 2014-06-02 22:09:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9920)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/11890)

[commit-bot: I haz the power, 2014-06-02 22:53:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:53:45]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9939)

[Manuel Rego, 2014-06-03 08:05:42]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-06-03 08:06:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rego@igalia.com/196943026/180001

[commit-bot: I haz the power, 2014-06-03 11:05:06]

Change committed as 175343