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

Julien Chaffraix

Reported 2013-02-13 10:51:21 PST

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

Comment 1 2013-02-13 11:07:42 PST

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

Julien Chaffraix

Comment 2 2013-02-15 13:33:07 PST

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

Ojan Vafai

Comment 3 2013-02-15 13:51:10 PST

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 4 2013-02-15 13:57:25 PST

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 5 2013-02-18 07:50:45 PST

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

Comment 6 2013-02-18 07:57:25 PST

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

WebKit Review Bot

Comment 7 2013-02-18 15:17:32 PST

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

Comment 8 2013-02-18 15:17:36 PST

All reviewed patches have been landed. Closing bug.

Attachments
Proposed patch: Split the code between resolution from style and cached position. (10.41 KB, patch) 2013-02-15 13:33 PST, Julien Chaffraix	no flags	Details Formatted Diff Diff
Updated patch for an EWS run. (12.71 KB, patch) 2013-02-18 07:57 PST, Julien Chaffraix	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.