[webkit-reviews] review granted: [Bug 109718] [CSS Grid Layout] Refactor grid position resolution code to support an internal grid representation : [Attachment 188634] Proposed patch: Split the code between resolution from style and cached position.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 109718: [CSS Grid Layout] Refactor grid position resolution code to support
an internal grid representation
https://bugs.webkit.org/show_bug.cgi?id=109718

Attachment 188634: Proposed patch: Split the code between resolution from style
and cached position.
https://bugs.webkit.org/attachment.cgi?id=188634&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188634&action=review


> Source/WebCore/rendering/RenderGrid.cpp:203
> +    m_gridItemsPosition.clear();

m_gridItemCoordinates would be more consistent with the fact that we're storing
GridCoordinates.

> Source/WebCore/rendering/RenderGrid.cpp:540
>      m_grid.clear();
> +    m_gridItemsPosition.clear();

Now that there's two things we need to do to clear, and with my patch that just
landed it will need a const_cast, maybe we should move this into a helper
function.

> Source/WebCore/rendering/RenderGrid.cpp:543
> +size_t RenderGrid::cachedGridPosition(TrackSizingDirection direction, const
RenderBox* gridItem) const

How about having a function that returns the GridCoordinates and then using the
GridCoordinates in all the calling locations?

Also, now that we have GridPosition and GridCoordinates, this should probably
be called cachedGridCoordinates.