Bug 153868 – [css-grid] GridSpan refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

This is somehow a port of a patch in Blink: https://codereview.chromium.org/1459373002/

It's not exactly the same, as we were already not using pointers in GridCoordinate in WebKit.
But porting this will help to avoid repeated calls to resolveGridPositionsFromStyle() from RenderGrid.

[Manuel Rego Casasnovas]

Created attachment 270656 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 270656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270656&action=review

> Source/WebCore/ChangeLog:8
> +        Add new boolean to know if a GridSpan is definite or indefinite.

I was going to complain asking for an enumerated type, but then I saw that you are actually using it. In any case you need to fix this sentence.

> Source/WebCore/rendering/style/GridCoordinate.h:50
> +

Extra line here

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:79
> +    initialPosition = (direction == ForColumns) ? gridItem.style().gridItemColumnStart() : gridItem.style().gridItemRowStart();

Consider doing
bool isRowAxis = direction == ForColumns;

and using that in these two ternary operators (also you don't need parens).

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:311
> +        // We can't get our grid positions without running the auto placement algorithm.

You can move the comment out of the if block and remove the brackets.

[Manuel Rego Casasnovas]

Created attachment 271549 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review, applied suggested changes!

[WebKit Commit Bot]

Comment on attachment 271549 [details]
Patch

Clearing flags on attachment: 271549

Committed r196691: <http://trac.webkit.org/changeset/196691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.