Bug 109718

Summary: [CSS Grid Layout] Refactor grid position resolution code to support an internal grid representation
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 103316    
Attachments:
Description Flags
Proposed patch: Split the code between resolution from style and cached position.
none
Updated patch for an EWS run. none

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.
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
DetailsFormatted DiffDiff
Updated patch for an EWS run. (12.71 KB, patch)
2013-02-18 07:57 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
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.
Note You need to log in before you can comment on or make changes to this bug.