[CSS Grid Layout] Fix issues adding new items to grid

[CSS Grid Layout] Fix issues adding new items to grid

First if the grid auto flow property is different to none, the grid is
marked as dirty when a new child is added. Because of if it contains
auto-placed items, they might need to be re-positioned.

Then if the new item has definite positions and auto flow is none, the
grid will grow as required and place the new item without being marked
as dirty.

The test was not working properly, as it was adding the PASS messages
as auto-placed grid items which was marking the grid as dirty and force
a re-calculation of the position.

Moving the results of the test to a different div make the test fails
without this patch. Test expectation was updated accordingly.

BUG=248151
TEST=fast/css-grid-layout/grid-item-addition-auto-placement-update.html

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

[Manuel Rego, 2014-03-20 17:10:09]

Uploaded new version simplifying RenderGrid::addChild() code.

Also improved the description of the code review.

[Julien - ping for review, 2014-03-21 04:10:35]

lgtm.

Some comments about the description:
* typo: diffent in the first paragraph.
* "The test was not working properly actually" <- this sounds weird, I would put
either properly or actually but not 2 adverbs next to each other.

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

https://codereview.chromium.org/198703004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:206: return;
Not sure what the return brings us here as it really was an else below :-)

https://codereview.chromium.org/198703004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:213: growGrid(ForColumns);
I have been thinking that we need a way to grow the grid more than a single item
at a time. This is for efficiency as well as to support auto-placement with
spanning items.

[Manuel Rego, 2014-03-21 11:26:48]

Thanks for the review.

On 2014/03/21 04:10:35, Julien Chaffraix - PST wrote:
> Some comments about the description:
> * typo: diffent in the first paragraph.
> * "The test was not working properly actually" <- this sounds weird, I would
put
> either properly or actually but not 2 adverbs next to each other.

Fixed.

>
https://codereview.chromium.org/198703004/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:206: return;
> Not sure what the return brings us here as it really was an else below :-)

Nothing special, but I like more the final result of the code, so if
you don't oppose I'll keep it like it is.

>
https://codereview.chromium.org/198703004/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:213: growGrid(ForColumns);
> I have been thinking that we need a way to grow the grid more than a single
item
> at a time. This is for efficiency as well as to support auto-placement with
> spanning items.

I agree. I've modified growGrid() adding a new param with the
positions to grow, so I'll wait for a new review for this change.

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

>
https://codereview.chromium.org/198703004/diff/20001/Source/core/rendering/Re...
> > Source/core/rendering/RenderGrid.cpp:206: return;
> > Not sure what the return brings us here as it really was an else below :-)
> 
> Nothing special, but I like more the final result of the code, so if
> you don't oppose I'll keep it like it is.

I won't oppose it. I do see an issue with not having an else if we ever add more
code after the now-implicit else. It also touches a lot of lines for no good
reason.

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

lgtm, I don't need to see the updated patch (unless you really want to me to!)

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

https://codereview.chromium.org/198703004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:209: // Grow grid if required.
Maybe we could turn that into a better comment:

// Ensure that the grid is big enough to contain new grid item.

https://codereview.chromium.org/198703004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:720: void
RenderGrid::growGrid(GridTrackSizingDirection direction, size_t positions)
How about we pass in the _maximum_ position we want the grid to be?

I think that would simplify the callers and not make this code worse. It is also
more aligned with what you expect of this API (you really don't care about the
delta size but about the maximum allowed position)

[Manuel Rego, 2014-03-22 00:04:57]

Applied suggested changes, thanks for the review.

[Manuel Rego, 2014-03-22 00:05:05]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-03-22 00:05:14]

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

[commit-bot: I haz the power, 2014-03-22 04:27:59]

Change committed as 169784