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

Description Manuel Rego Casasnovas 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.
Comment 1 Manuel Rego Casasnovas 2014-07-11 13:58:59 PDT
Created attachment 234778 [details]
Patch
Comment 2 Sergio Villar Senin 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.
Comment 3 Manuel Rego Casasnovas 2014-07-14 13:44:39 PDT
Created attachment 234876 [details]
Patch for landing
Comment 4 WebKit Commit Bot 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>
Comment 5 WebKit Commit Bot 2014-07-14 14:24:15 PDT
All reviewed patches have been landed.  Closing bug.