Bug 91293 – Position grid items by row/column index

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Position grid items by row/column index

[Tony Chang]

Created attachment 152363 [details]
Patch

[Tony Chang]

First patch for grid layout!  It's very basic and only handles fixed size tracks and doesn't resize grid items.

[Gustavo Noronha (kov)]

Comment on attachment 152363 [details]
Patch

Attachment 152363 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13235287

[Early Warning System Bot]

Comment on attachment 152363 [details]
Patch

Attachment 152363 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13237291

[Tony Chang]

Created attachment 152377 [details]
Patch

[Ojan Vafai]

Comment on attachment 152377 [details]
Patch

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?

[Tony Chang]

Created attachment 152574 [details]
Patch for landing

[Tony Chang]

Comment on attachment 152377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152377&action=review

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

We have to handle it in both places and I didn't want to forget.

>> Source/WebCore/rendering/RenderGrid.cpp:159
>> +    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?

Right, it shouldn't be a Length, but that's what we're currently using to hold auto or integer.  We can fix this in a follow up (once we can parse more than just auto and ints.

>> Source/WebCore/rendering/RenderGrid.cpp:169
>> +    size_t rowTrack = static_cast<size_t>(row.getFloatValue()) - 1;
> 
> These should just use intValue() and ints instead of size_t's, no?

Switched to intValue, but still using size_t because {column,row}Tracks.size() returns a size_t.  These should both be unsigned since we checked isPositive above.

[WebKit Review Bot]

Comment on attachment 152574 [details]
Patch for landing

Clearing flags on attachment: 152574

Committed r122747: <http://trac.webkit.org/changeset/122747>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.