[webkit-reviews] review granted: [Bug 91293] Position grid items by row/column index : [Attachment 152377] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Tony Chang <tony at chromium.org>'s
request for review:
Bug 91293: Position grid items by row/column index
https://bugs.webkit.org/show_bug.cgi?id=91293

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

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


Looks great! Just a bunch of request for FIXMEs. :)

> Source/WebCore/rendering/RenderGrid.cpp:43
> +    LayoutUnit m_usedBreadth;

I'm not a huge fan of usedBreadth for this. How about just m_size? Then you'd
have computeSizeOfGridTracks, which seems very natural to me.

Ugh, I see now that this is matching the terminology in the spec. I suppose
that makes sense and will make development/maintenance easier. Can you add a
comment to that effect in the ChangeLog?

> Source/WebCore/rendering/RenderGrid.cpp:57
> +void RenderGrid::layoutBlock(bool relayoutChildren, LayoutUnit)

Lol. This looks familiar. At some point we should step back and see if
RenderFlexibleBox and RenderGrid could share more of this code with
RenderBlock. There's just too much boilerplate. Add a FIXME?

> Source/WebCore/rendering/RenderGrid.cpp:81
> +    // For overflow:scroll blocks, ensure we have both scrollbars in place
always.

Meh to this useless comment.

> Source/WebCore/rendering/RenderGrid.cpp:148
> +

Extra line break.

> Source/WebCore/rendering/RenderGrid.cpp:151
> +    // FIXME: Handle border & padding on the grid element.

Duplicated comment with line 146.

> Source/WebCore/rendering/RenderGrid.cpp:159
> +    Length column = child->style()->gridItemColumn();
> +    Length row = child->style()->gridItemRow();

Length doesn't seem like the right type for these. From the spec:
[[<integer>|<string>|start][<integer>|<string>|end]?]|auto.

Not sure what the type should be, but it's certainly not Length. We might need
to define a new type. Add a FIXME?

> Source/WebCore/rendering/RenderGrid.cpp:161
> +    if (!column.isPositive() || !row.isPositive())

The spec doesn't seem to cover what to do with non-positive values, but
specifies the type as integer...so non-positive is allowed. Not sure if there's
a use-case. Either way, we should bring it up with the CSSWG. If non-positive
isn't allowed, we should be clamping these to 1 in the CSSParser, right? For
now, can you add a FIXME?

> Source/WebCore/rendering/RenderGrid.cpp:169
> +    size_t columnTrack = static_cast<size_t>(column.getFloatValue()) - 1;
> +    size_t rowTrack = static_cast<size_t>(row.getFloatValue()) - 1;

These should just use intValue() and ints instead of size_t's, no?