Bug 134544 – [CSS Grid Layout] Support sparse in auto-placement algorithm

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 234778 [details]
Patch

[Sergio Villar Senin]

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]

Created attachment 234876 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 234876 [details]
Patch for landing

Clearing flags on attachment: 234876

Committed r171082: <http://trac.webkit.org/changeset/171082>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.