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.