Bug 153868 - [css-grid] GridSpan refactoring
Summary: [css-grid] GridSpan refactoring
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
Depends on:
Blocks: 153488
  Show dependency treegraph
Reported: 2016-02-04 08:01 PST by Manuel Rego Casasnovas
Modified: 2016-02-17 03:45 PST (History)
4 users (show)

See Also:

Patch (50.50 KB, patch)
2016-02-04 08:04 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (50.46 KB, patch)
2016-02-17 02:54 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Sergio Villar Senin 2016-02-17 01:39:01 PST
Comment on attachment 270656 [details]

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]
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]

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.