[CSS Grid Layout] Refactor grid position resolution code to support an internal grid representation

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently we always re-resolve the grid computed position when we iterate over all our children. 

As computing the 2D grid will be expensive, we would rather reuse the grid items' position information from the grid instead of recomputing it every time as we do (this will be even more true when we add support for grid-auto-flow as this is not an answer that can be inferred from the style anymore).

We have 2 variants of resolveGridPosition that do the same thing but could be updated to return different results based on whether we use our cached grid or not.

[Julien Chaffraix]

Scratch that, this refactoring would make sense *after* we land a grid representation.

[Julien Chaffraix]

Created attachment 188634 [details]
Proposed patch: Split the code between resolution from style and cached position.

[Ojan Vafai]

Comment on attachment 188634 [details]
Proposed patch: Split the code between resolution from style and cached position.

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.

[Tony Chang]

Comment on attachment 188634 [details]
Proposed patch: Split the code between resolution from style and cached position.

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

>> Source/WebCore/rendering/RenderGrid.cpp:540
>> +    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.

Might also want to make an RAII class that populates and clears when it goes out of scope, but that would be a separate patch.

> Source/WebCore/rendering/RenderGrid.h:53
> +    struct GridCoordinates {

Nit: I would name the struct GridCoordinate since it's a single coordinate.

[Julien Chaffraix]

Comment on attachment 188634 [details]
Proposed patch: Split the code between resolution from style and cached position.

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.

OK.

>>> Source/WebCore/rendering/RenderGrid.cpp:540
>>> +    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.
> 
> Might also want to make an RAII class that populates and clears when it goes out of scope, but that would be a separate patch.

I am not entirely convinced we should do it *now*. If the code complexity increases (which will happen as we support more of the specification) and more branches are added, maybe it would make sense. Also layout methods do not have early returns which makes a RAII pattern an overkill. I will file a bug about it to track that.

>> 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.

Sounds good.

>> Source/WebCore/rendering/RenderGrid.h:53
>> +    struct GridCoordinates {
> 
> Nit: I would name the struct GridCoordinate since it's a single coordinate.

OK.

[Julien Chaffraix]

Created attachment 188889 [details]
Updated patch for an EWS run.

[WebKit Review Bot]

Comment on attachment 188889 [details]
Updated patch for an EWS run.

Clearing flags on attachment: 188889

Committed r143268: <http://trac.webkit.org/changeset/143268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.