Bug 141748 – [CSS Grid Layout] Support "sparse" in auto-placed items locked to a row/column

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

[Manuel Rego Casasnovas]

The spec has been updated and now sparse should be supported for items fixed to a given row (or column):
http://dev.w3.org/csswg/css-grid/#auto-placement-algo

[Manuel Rego Casasnovas]

Created attachment 246807 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 246807 [details]
Patch

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

The patch looks good. 

I'm giving r- just because I think the implementation using a hash (see comments inline) deserves a try.

> Source/WebCore/rendering/RenderGrid.cpp:882
> +    Vector<size_t> minorAxisCursors;

It should be <unsigned>

> Source/WebCore/rendering/RenderGrid.cpp:886
> +    }

It seems to me that having a vector here is perhaps not very optimal memory-wise because I have the impression that most of them would be actually empty (0 in this case). From that POV a hash would be perhaps better, it won't require the initialization phase either although searches would be slower. Could you check the performance of that?

> Source/WebCore/rendering/RenderGrid.cpp:900
> +            minorAxisCursors[majorAxisPositions->resolvedInitialPosition.toInt()] = (direction == ForColumns) ? emptyGridArea->rows.resolvedInitialPosition.toInt() : emptyGridArea->columns.resolvedInitialPosition.toInt();

As direction is not going to change, perhaps we better store (direction == ForColumns) in a variable at the beginning of the function and use it here.

[Manuel Rego Casasnovas]

Created attachment 247144 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to comment #2)
> > Source/WebCore/rendering/RenderGrid.cpp:882
> > +    Vector<size_t> minorAxisCursors;
> 
> It should be <unsigned>

Right, I've changed it.

> > Source/WebCore/rendering/RenderGrid.cpp:886
> > +    }
> 
> It seems to me that having a vector here is perhaps not very optimal
> memory-wise because I have the impression that most of them would be
> actually empty (0 in this case). From that POV a hash would be perhaps
> better, it won't require the initialization phase either although searches
> would be slower. Could you check the performance of that?

I agree that this is not going to be the default case, actually most of the times it'll be an exception. So, for big grid avoiding the Vector might be a good idea.

I've been testing with HashMap vs Vector in a 200x200 grid, and the performance results show that the HashMap is slightly faster.

On top of that, IMHO the code is clearer with the map, so I'm uploading a new patch using a map instead of a vector.

> > Source/WebCore/rendering/RenderGrid.cpp:900
> > +            minorAxisCursors[majorAxisPositions->resolvedInitialPosition.toInt()] = (direction == ForColumns) ? emptyGridArea->rows.resolvedInitialPosition.toInt() : emptyGridArea->columns.resolvedInitialPosition.toInt();
> 
> As direction is not going to change, perhaps we better store (direction ==
> ForColumns) in a variable at the beginning of the function and use it here.

Done.

[Sergio Villar Senin]

Comment on attachment 247144 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:879
> +    bool isForColumns = (autoPlacementMajorAxisDirection() == ForColumns);

No need for parentheses.

> Source/WebCore/rendering/RenderGrid.cpp:891
> +        unsigned minorAxisCursorPosition = !isGridAutoFlowDense && minorAxisCursors.contains(majorAxisInitialPosition) ? minorAxisCursors.get(majorAxisInitialPosition) : 0;

This doesn't look optimal, you're querying the hashmap twice.

[Manuel Rego Casasnovas]

Created attachment 247214 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #5)
> Comment on attachment 247144 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247144&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:879
> > +    bool isForColumns = (autoPlacementMajorAxisDirection() == ForColumns);
> 
> No need for parentheses.

Sure.

> > Source/WebCore/rendering/RenderGrid.cpp:891
> > +        unsigned minorAxisCursorPosition = !isGridAutoFlowDense && minorAxisCursors.contains(majorAxisInitialPosition) ? minorAxisCursors.get(majorAxisInitialPosition) : 0;
> 
> This doesn't look optimal, you're querying the hashmap twice.

Yeah, you're right, actually it's enough with using .get() as it'll return 0 for the case of unsiged if it's not in the map yet.

[Sergio Villar Senin]

Comment on attachment 247214 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:888
> +        unsigned majorAxisInitialPosition = majorAxisPositions->resolvedInitialPosition.toInt() + 1;

Instead of this better use UnsignedWithZeroKeyHashTraits in the definition of the map and...

> Source/WebCore/rendering/RenderGrid.cpp:890
> +        GridIterator iterator(m_grid, autoPlacementMajorAxisDirection(), majorAxisPositions->resolvedInitialPosition.toInt(), isGridAutoFlowDense ? 0 : minorAxisCursors.get(majorAxisInitialPosition));

... you'd have to use find() here instead of get to check whether or not the cursor exists.

[Manuel Rego Casasnovas]

Created attachment 247216 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the fast reviews!

(In reply to comment #8)
> > Source/WebCore/rendering/RenderGrid.cpp:888
> > +        unsigned majorAxisInitialPosition = majorAxisPositions->resolvedInitialPosition.toInt() + 1;
> 
> Instead of this better use UnsignedWithZeroKeyHashTraits in the definition
> of the map and...

True. Thanks for the pointer.

> > Source/WebCore/rendering/RenderGrid.cpp:890
> > +        GridIterator iterator(m_grid, autoPlacementMajorAxisDirection(), majorAxisPositions->resolvedInitialPosition.toInt(), isGridAutoFlowDense ? 0 : minorAxisCursors.get(majorAxisInitialPosition));
> 
> ... you'd have to use find() here instead of get to check whether or not the
> cursor exists.

Actually as I'm just using UnsignedWithZeroKeyHashTraits for they key, I don't need to use find() and can keep get() which makes the code simpler.

New version with the suggested changes uploaded.

[Sergio Villar Senin]

Comment on attachment 247216 [details]
Patch

Yay! much better. Ideally we would have some description before the test cases in order to know what are we really testing, because it's a bit difficult to know just taking a look at the HTML.

[Manuel Rego Casasnovas]

(In reply to comment #11)
> Comment on attachment 247216 [details]
> Patch
> 
> Yay! much better. Ideally we would have some description before the test
> cases in order to know what are we really testing, because it's a bit
> difficult to know just taking a look at the HTML.

I don't see an easy way, without entering in lots of details in the comments, which will make them useless and it'll be simpler to take a look to the HTML.

In addition, we've several tests with this very same format.

So, I'm landing this as it's and we could add the comments in a follow-up patch if we find a good way.

[WebKit Commit Bot]

Comment on attachment 247216 [details]
Patch

Clearing flags on attachment: 247216

Committed r180567: <http://trac.webkit.org/changeset/180567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.