Bug 91293 - Position grid items by row/column index
Summary: Position grid items by row/column index
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2012-07-13 15:44 PDT by Tony Chang
Modified: 2012-07-16 12:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (16.25 KB, patch)
2012-07-13 16:06 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2012-07-13 16:57 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (16.45 KB, patch)
2012-07-16 11:37 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-07-13 15:44:44 PDT
Position grid items by row/column index
Comment 1 Tony Chang 2012-07-13 16:06:47 PDT
Created attachment 152363 [details]
Patch
Comment 2 Tony Chang 2012-07-13 16:07:52 PDT
First patch for grid layout!  It's very basic and only handles fixed size tracks and doesn't resize grid items.
Comment 3 Gustavo Noronha (kov) 2012-07-13 16:25:09 PDT
Comment on attachment 152363 [details]
Patch

Attachment 152363 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13235287
Comment 4 Early Warning System Bot 2012-07-13 16:46:54 PDT
Comment on attachment 152363 [details]
Patch

Attachment 152363 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13237291
Comment 5 Tony Chang 2012-07-13 16:57:02 PDT
Created attachment 152377 [details]
Patch
Comment 6 Ojan Vafai 2012-07-13 19:17:24 PDT
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?
Comment 7 Tony Chang 2012-07-16 11:37:03 PDT
Created attachment 152574 [details]
Patch for landing
Comment 8 Tony Chang 2012-07-16 11:41:18 PDT
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.
Comment 9 WebKit Review Bot 2012-07-16 12:10:10 PDT
Comment on attachment 152574 [details]
Patch for landing

Clearing flags on attachment: 152574

Committed r122747: <http://trac.webkit.org/changeset/122747>
Comment 10 WebKit Review Bot 2012-07-16 12:10:16 PDT
All reviewed patches have been landed.  Closing bug.