[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 bugzilla-daemon at webkit.org
Fri Jun 27 07:43:13 PDT 2014

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

Attachment 233690: Patch

------- 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

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,

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.

> +    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.


> +    background-color: maroon;

Same comment here about colors.

More information about the webkit-reviews mailing list