Bug 102441 - RenderGrid should have a function to resolve grid position
Summary: RenderGrid should have a function to resolve grid position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2012-11-15 15:56 PST by Julien Chaffraix
Modified: 2012-11-16 05:32 PST (History)
5 users (show)

See Also:


Attachments
Proposed refactoring. (4.61 KB, patch)
2012-11-15 16:07 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.