Bug 141748 - [CSS Grid Layout] Support "sparse" in auto-placed items locked to a row/column
Summary: [CSS Grid Layout] Support "sparse" in auto-placed items locked to a row/column
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 103316
  Show dependency treegraph
 
Reported: 2015-02-18 00:56 PST by Manuel Rego Casasnovas
Modified: 2015-02-24 09:38 PST (History)
6 users (show)

See Also:


Attachments
Patch (24.45 KB, patch)
2015-02-18 01:00 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (24.81 KB, patch)
2015-02-23 13:56 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (24.62 KB, patch)
2015-02-24 02:18 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (24.69 KB, patch)
2015-02-24 03:08 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 2015-02-18 00:56:44 PST
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
Comment 1 Manuel Rego Casasnovas 2015-02-18 01:00:13 PST
Created attachment 246807 [details]
Patch
Comment 2 Sergio Villar Senin 2015-02-23 03:52:15 PST
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.
Comment 3 Manuel Rego Casasnovas 2015-02-23 13:56:39 PST
Created attachment 247144 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2015-02-23 14:00:09 PST
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.
Comment 5 Sergio Villar Senin 2015-02-24 00:16:44 PST
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.
Comment 6 Manuel Rego Casasnovas 2015-02-24 02:18:39 PST
Created attachment 247214 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2015-02-24 02:19:52 PST
(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.
Comment 8 Sergio Villar Senin 2015-02-24 02:54:12 PST
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.
Comment 9 Manuel Rego Casasnovas 2015-02-24 03:08:04 PST
Created attachment 247216 [details]
Patch
Comment 10 Manuel Rego Casasnovas 2015-02-24 03:10:34 PST
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.
Comment 11 Sergio Villar Senin 2015-02-24 08:00:53 PST
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.
Comment 12 Manuel Rego Casasnovas 2015-02-24 08:54:45 PST
(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.
Comment 13 WebKit Commit Bot 2015-02-24 09:38:21 PST
Comment on attachment 247216 [details]
Patch

Clearing flags on attachment: 247216

Committed r180567: <http://trac.webkit.org/changeset/180567>
Comment 14 WebKit Commit Bot 2015-02-24 09:38:26 PST
All reviewed patches have been landed.  Closing bug.