Bug 102441

Summary: RenderGrid should have a function to resolve grid position
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Proposed refactoring. none

Description Julien Chaffraix 2012-11-15 15:56:45 PST
Currently mapping grid-row or grid-column (grid-position-row and grid-position-col in the latest spec) to the actual position on the grid is done in an implicit way inside RenderGrid::findChildLogicalPosition.

In order to add support for grid areas, we need a way to get the grid items' explicit position which is where this function would come in handy.

Patch forthcoming.
Comment 1 Julien Chaffraix 2012-11-15 16:07:23 PST
Created attachment 174539 [details]
Proposed refactoring.
Comment 2 Ojan Vafai 2012-11-15 16:11:03 PST
Comment on attachment 174539 [details]
Proposed refactoring.

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

> Source/WebCore/rendering/RenderGrid.cpp:178
> +size_t RenderGrid::resolveGridPosition(const Length& position) const

Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit?
Comment 3 Julien Chaffraix 2012-11-15 16:20:47 PST
Comment on attachment 174539 [details]
Proposed refactoring.

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

>> Source/WebCore/rendering/RenderGrid.cpp:178
>> +size_t RenderGrid::resolveGridPosition(const Length& position) const
> 
> Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit?

OK, here are the answers in order:
* No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans.
* I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid.
* Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid.

If you have some suggestion on how to better convey this idea, I would love to update the patch.
Comment 4 Ojan Vafai 2012-11-15 16:26:59 PST
Comment on attachment 174539 [details]
Proposed refactoring.

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

>>> Source/WebCore/rendering/RenderGrid.cpp:178
>>> +size_t RenderGrid::resolveGridPosition(const Length& position) const
>> 
>> Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit?
> 
> OK, here are the answers in order:
> * No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans.
> * I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid.
> * Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid.
> 
> If you have some suggestion on how to better convey this idea, I would love to update the patch.

oic. It's confusing that we using Length to represent these. I think we need a new value type.
Comment 5 Julien Chaffraix 2012-11-15 16:36:33 PST
Comment on attachment 174539 [details]
Proposed refactoring.

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

>>>> Source/WebCore/rendering/RenderGrid.cpp:178
>>>> +size_t RenderGrid::resolveGridPosition(const Length& position) const
>>> 
>>> Isn't this the grid size? resolveGridExtent would be more consistent with what we call these in the rest of the codebase. Also, should this return a LayoutUnit?
>> 
>> OK, here are the answers in order:
>> * No, the grid size is the max of the grid items' grid-position-row and grid-position-column, including the spans.
>> * I don't think 'extend' is the right word here as it's not a sizing function: it is meant to return the physical position on the 2D grid.
>> * Returning LayoutUnit makes no sense, we can't place an item at the (1/60, 1/60) position on the grid.
>> 
>> If you have some suggestion on how to better convey this idea, I would love to update the patch.
> 
> oic. It's confusing that we using Length to represent these. I think we need a new value type.

I agree: I used Length as a temporary type (as it supports 'auto' and <integer>) until we actually get to the point where we require this new type. It is orthogonal to this refactoring though and I will be more than happy to clean this up in a follow-up bug.
Comment 6 WebKit Review Bot 2012-11-16 05:32:53 PST
Comment on attachment 174539 [details]
Proposed refactoring.

Clearing flags on attachment: 174539

Committed r134935: <http://trac.webkit.org/changeset/134935>
Comment 7 WebKit Review Bot 2012-11-16 05:32:56 PST
All reviewed patches have been landed.  Closing bug.