[webkit-reviews] review denied: [Bug 113546] [CSS Grid Layout] Align our grid-line handling with the updated specification : [Attachment 196193] Updated change: Same as previously but properly resolve against the explicit grid.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 113546: [CSS Grid Layout] Align our grid-line handling with the updated
specification
https://bugs.webkit.org/show_bug.cgi?id=113546

Attachment 196193: Updated change: Same as previously but properly resolve
against the explicit grid.
https://bugs.webkit.org/attachment.cgi?id=196193&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196193&action=review


Would like to understand why we're adding the member variables before r+.

> Source/WebCore/rendering/RenderGrid.cpp:348
> +void RenderGrid::computeExplicitGridSizes()
>  {
> -    const Vector<GridTrackSize>& trackStyles = (direction == ForColumns) ?
style()->gridColumns() : style()->gridRows();
> +    m_explicitGridRowCount = style()->gridRows().size();
> +    m_explicitGridColumnCount = style()->gridColumns().size();
> +}

Why do we need this? Why not just use style()->gridRows().size() and
style()->gridColumns().size() directly?

> Source/WebCore/rendering/RenderGrid.cpp:783
> +	   // FIXME: This returns one less than the expected result for side ==
StartSide or BeforeSide as we don't properly convert
> +	   // the grid line to its grid track. However this avoids the issue of
growing the grid when inserting the item (e.g. -1 / auto).

I now understand this...I think the spec is dumb. I've nagged Tab and fantasai
about this on IRC. In either case, this is fine for now.