[webkit-reviews] review granted: [Bug 110633] [CSS Grid Layout] Interaction between auto-placement and column / row spanning : [Attachment 233690] Patch

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 110633: [CSS Grid Layout] Interaction between auto-placement and column /
row spanning
https://bugs.webkit.org/show_bug.cgi?id=110633

Attachment 233690: Patch
https://bugs.webkit.org/attachment.cgi?id=233690&action=review

------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233690&action=review


Awesome job. Please fix the nits before landing.

> Source/WebCore/ChangeLog:44
> +	   (WebCore::RenderGrid::growGrid): Deleted. Renamed to
ensureGridSize().

Nit no need for Deleted. It's more than enough with Renamed.

> Source/WebCore/rendering/RenderGrid.cpp:120
> +    bool checkEmptyCells(size_t rowSpan, size_t columnSpan) const

I don't like the function name, in this case it should be something like
hasEnoughRoomForArea() or isEmptyAreaEnoughForItem() for example. Function name
should start with isXXX() or hasXXX() but definitely not checkXXX().

> Source/WebCore/rendering/RenderGrid.cpp:124
> +	   size_t maxColumns = std::min(m_columnIndex + columnSpan,
m_grid[0].size());

Better use gridColumnCount() and gridRowCount().

> Source/WebCore/rendering/RenderGrid.cpp:149
>	   const size_t endOfVaryingTrackIndex = (m_direction == ForColumns) ?
m_grid.size() : m_grid[0].size();

I know it was not modified by this patch but use gridColumnCount() and
gridRowCount() also here.

> Source/WebCore/rendering/RenderGrid.cpp:635
> +    const size_t oldRowSize = gridRowCount();

Let's use oldColumnCount and oldRowCount for consistency.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:65
> +

Nit: extra line here.

>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html:1
9
> +    background-color: maroon;

I'm sure it was useful for testing :) but let's remove the background colors as
they're not needed by the test.

>
LayoutTests/fast/css-grid-layout/grid-item-auto-placement-definite-span.html:11

> +    background-color: maroon;

Same comment here about colors.