[webkit-reviews] review granted: [Bug 110277] [CSS Grid Layout] Implement the auto-placement algorithm without grid growth : [Attachment 189200] Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 110277: [CSS Grid Layout] Implement the auto-placement algorithm without
grid growth
https://bugs.webkit.org/show_bug.cgi?id=110277

Attachment 189200: Proposed patch: Change is big due to testing and scaffolding
code. Splittable on demand.
https://bugs.webkit.org/attachment.cgi?id=189200&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=189200&action=review


This is pretty straightforward.

> Source/WebCore/ChangeLog:16
> +	   are no empty grid area. If we don't find any empty grid area, we
just insert in the first

Nit: area -> areas (both uses).

> Source/WebCore/rendering/RenderGrid.cpp:100
> +    PassOwnPtr<GridCoordinate> firstEmptyGridArea()

Nit: I think nextEmptyGridArea would be more clear.  first sounds like you're
resetting the value.

> Source/WebCore/rendering/RenderGrid.cpp:102
> +	   if (!m_grid.size())

Nit: m_grid.isEmpty()

> Source/WebCore/rendering/RenderGrid.cpp:534
> +    placeDefinedMajorAxisItemsOnGrid(autoGridItems);
> +    placeAutoMajorAxisItemsOnGrid(autoGridItems);

It's a bit unfortunate that we have to loop through autoGridItems twice.  Maybe
we should have 2 vectors, one for defined and one for auto?

>
LayoutTests/fast/css-grid-layout/grid-item-addition-auto-placement-update-expec
ted.txt:14
> +FAIL:
> +Expected 30 for height, but got 50. 

Can you add some comments next to the test cases that fail (in the .html file)
explaining the failures?  These don't have to show up in the expected file.

>
LayoutTests/fast/css-grid-layout/grid-item-removal-auto-placement-update-expect
ed.txt:5
> +FAIL:
> +Expected 170 for width, but got 50. 

Comments for these failures too.