Bug 153868

Summary: [css-grid] GridSpan refactoring
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfernandez, simon.fraser, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 153488    
Attachments:
Description Flags
Patch
none
Patch none

Description Manuel Rego Casasnovas 2016-02-04 08:01:10 PST
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.
Comment 1 Manuel Rego Casasnovas 2016-02-04 08:04:29 PST
Created attachment 270656 [details]
Patch
Comment 2 Sergio Villar Senin 2016-02-17 01:39:01 PST
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.
Comment 3 Manuel Rego Casasnovas 2016-02-17 02:54:31 PST
Created attachment 271549 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2016-02-17 02:55:38 PST
Thanks for the review, applied suggested changes!
Comment 5 WebKit Commit Bot 2016-02-17 03:45:29 PST
Comment on attachment 271549 [details]
Patch

Clearing flags on attachment: 271549

Committed r196691: <http://trac.webkit.org/changeset/196691>
Comment 6 WebKit Commit Bot 2016-02-17 03:45:33 PST
All reviewed patches have been landed.  Closing bug.