Bug 134544

Summary: [CSS Grid Layout] Support sparse in auto-placement algorithm
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, jfernandez, kondapallykalyan, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 134057    
Bug Blocks: 103316, 134842    
Attachments:
Description Flags
Patch
none
Patch for landing none

Manuel Rego Casasnovas
Reported 2014-07-02 04:41:13 PDT
Once the new grid-auto-flow syntax is ready (bug #134057) we should implement proper support for sparse mode in auto-placement algorithm. This is being addressed in Blink (https://codereview.chromium.org/362733002/) and should be ported to WebKit.
Attachments
Patch (52.09 KB, patch)
2014-07-11 13:58 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch for landing (52.01 KB, patch)
2014-07-14 13:44 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2014-07-11 13:58:59 PDT
Created attachment 234778 [details] Patch
Sergio Villar Senin
Comment 2 2014-07-14 09:55:16 PDT
Comment on attachment 234778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234778&action=review Awesome patch! As usual :) some comments. > Source/WebCore/ChangeLog:8 > + Sparse mode for auto-placement algorithm was not implemented yet. Remove this line, it adds nothing. > Source/WebCore/ChangeLog:10 > + This patch implements sparse mode for auto-placement algorithm. It keeps Let's explain that the algorithm is "sparse" by default, otherwise people might get confused as the change does not add parsing for any new keyword. > Source/WebCore/ChangeLog:14 > + If we're in dense mode it resets the cursor after each item. Maybe add some text here mentioning that the old code was using the dense mode by default. Otherwise it looks like we're implementing both sparse and dense. > Source/WebCore/rendering/RenderGrid.cpp:764 > + // If grid-auto-flow is dense, reset auto-placement cursor. Let's remove this comment. The code is totally self-explanatory. > Source/WebCore/rendering/RenderGrid.cpp:765 > + if (style().isGridAutoFlowAlgorithmDense()) { Actually there is no need to call it for every single item, we can store the value in a var outside the for loop and use it here. > Source/WebCore/rendering/RenderGrid.cpp:772 > +void RenderGrid::placeAutoMajorAxisItemOnGrid(RenderBox* gridItem, std::pair<size_t, size_t>& autoPlacementCursor) I think it'd be a good idea to give the std::pair a good type name and use it instead. AutoPlacementCursor sounds good to me :) > Source/WebCore/rendering/RenderGrid.cpp:823 > + // Move auto-placement cursor to the new position. No need for this comment. > Source/WebCore/rendering/RenderGrid.h:80 > + void placeAutoMajorAxisItemOnGrid(RenderBox*, std::pair<size_t, size_t>& autoPlacementCursor); Ditto the std::pair. Note that the argument name won't be needed.
Manuel Rego Casasnovas
Comment 3 2014-07-14 13:44:39 PDT
Created attachment 234876 [details] Patch for landing
WebKit Commit Bot
Comment 4 2014-07-14 14:24:12 PDT
Comment on attachment 234876 [details] Patch for landing Clearing flags on attachment: 234876 Committed r171082: <http://trac.webkit.org/changeset/171082>
WebKit Commit Bot
Comment 5 2014-07-14 14:24:15 PDT
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.