Summary: | RenderGrid should have a function to resolve grid position | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||
Component: | Layout and Rendering | Assignee: | 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
Julien Chaffraix
2012-11-15 15:56:45 PST
Created attachment 174539 [details]
Proposed refactoring.
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 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 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 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 on attachment 174539 [details] Proposed refactoring. Clearing flags on attachment: 174539 Committed r134935: <http://trac.webkit.org/changeset/134935> All reviewed patches have been landed. Closing bug. |