[webkit-reviews] review granted: [Bug 134544] [CSS Grid Layout] Support sparse in auto-placement algorithm : [Attachment 234778] Patch

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 134544: [CSS Grid Layout] Support sparse in auto-placement algorithm
https://bugs.webkit.org/show_bug.cgi?id=134544

Attachment 234778: Patch
https://bugs.webkit.org/attachment.cgi?id=234778&action=review

------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
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.