Bug 148944 – [CSS Grid Layout] Flex tracks sizing alg must handle 0fr values

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

[Javier Fernandez]

We don't allow 0 as flexible size value, which is not following current specs; it just states that it must be a positive value. 

Besides, we don't manage properly fraction units when using indefinite size for the grid container. We could use this opportunity to implement the new algorithm for determining the flexible tracks sizes, because we are still using the old one.

[Javier Fernandez]

Created attachment 260884 [details]
Patch

[Darin Adler]

Comment on attachment 260884 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        We don't allow 0 as flexible size value, which is not following current
> +        specs; it just states that it must be a positive value. This patch
> +        adds such change in the parser but some additional logic must be added
> +        as well to handle 0 values during the flex tracks sizing algorithm.

Sorry to be pedantic, but 0 is not a positive value. Often the term nonnegative is used instead of positive to clarify that 0 is allowed. Might be nice to fix the spec language if it’s treating 0 as a positive value.

> Source/WebCore/rendering/RenderGrid.cpp:455
> +    Vector<unsigned> flexibleSizedTracksIndex;

This is a case where adding some inline capacity can improve performance if the vector is normally small. Given that this is a leaf function, I think we could write something like Vector<unsigned, 256>.

> Source/WebCore/rendering/RenderGrid.cpp:476
> +            tracksToTreatAsInflexible.add(trackIndex);

What guarantees that trackIndex won’t be 0xFFFFFFFE or 0xFFFFFFFF? If we can’t guarantee that, then it’s not safe to call add here.

> Source/WebCore/rendering/RenderGrid.cpp:481
> +    return succeed ? hypotheticalFrSize : findFrUnitSize(tracks, tracksSpan, direction, spaceToFill, tracksToTreatAsInflexible);

Is this tail recursion going to be efficient? Can we write this algorithm in a way that’s iterative instead of recursive?

> Source/WebCore/rendering/RenderGrid.h:115
> +    double findFrUnitSize(Vector<GridTrack>&, const GridSpan&, GridTrackSizingDirection, LayoutUnit spaceToFill, TrackIndexSet tracksToTreatAsInflexible = TrackIndexSet()) const;

We would get better performance if we passed in a TrackIndexSet& instead of copying the set every time we make the call. It might be tricky to do it just right, though. It would be really nice not to copy a HashSet every time we recurse, since that’s a non-trivial operation.

Is the right name for this “find fr unit size”? Can we give it a name that doesn’t use that abbreviation, or is “fr” a term of art here?

[Javier Fernandez]

Comment on attachment 260884 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        as well to handle 0 values during the flex tracks sizing algorithm.
> 
> Sorry to be pedantic, but 0 is not a positive value. Often the term nonnegative is used instead of positive to clarify that 0 is allowed. Might be nice to fix the spec language if it’s treating 0 as a positive value.

Hey, np. Actually, spec talks about "non-negative" value so this comment is totally right. I'll modify the ChangeLog test accordingly.

>> Source/WebCore/rendering/RenderGrid.cpp:481
>> +    return succeed ? hypotheticalFrSize : findFrUnitSize(tracks, tracksSpan, direction, spaceToFill, tracksToTreatAsInflexible);
> 
> Is this tail recursion going to be efficient? Can we write this algorithm in a way that’s iterative instead of recursive?

I think it's a good idea, I'll try to find an iterative equivalent logic.

>> Source/WebCore/rendering/RenderGrid.h:115
>> +    double findFrUnitSize(Vector<GridTrack>&, const GridSpan&, GridTrackSizingDirection, LayoutUnit spaceToFill, TrackIndexSet tracksToTreatAsInflexible = TrackIndexSet()) const;
> 
> We would get better performance if we passed in a TrackIndexSet& instead of copying the set every time we make the call. It might be tricky to do it just right, though. It would be really nice not to copy a HashSet every time we recurse, since that’s a non-trivial operation.
> 
> Is the right name for this “find fr unit size”? Can we give it a name that doesn’t use that abbreviation, or is “fr” a term of art here?

Even that my goal would be to find an iterative version of this function, I'll use the reference anyway as you suggested.  Changing the function name is indeed a good idea as well.

[Javier Fernandez]

Comment on attachment 260884 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:455
>> +    Vector<unsigned> flexibleSizedTracksIndex;
> 
> This is a case where adding some inline capacity can improve performance if the vector is normally small. Given that this is a leaf function, I think we could write something like Vector<unsigned, 256>.

I've been thinking about this and it's a good idea, indeed, but perhaps 256 is too much. I'll set the inline capacity to 8, since we are only dealing here with Flexible tracks and we don't expect many of them in general grid definitions.

>> Source/WebCore/rendering/RenderGrid.cpp:476
>> +            tracksToTreatAsInflexible.add(trackIndex);
> 
> What guarantees that trackIndex won’t be 0xFFFFFFFE or 0xFFFFFFFF? If we can’t guarantee that, then it’s not safe to call add here.

Those indexes are retrieved from a GridSpan structure, which has many assertions verifying that they are non-negative integers. See WebCore/rendering/style/GridResolvedPosition.[cpp, h] for details.

[Javier Fernandez]

Created attachment 261838 [details]
Patch

[Javier Fernandez]

Even that we had r+ on the previous patch, I've added a new one addressing some of the issues commented in #c2

[Javier Fernandez]

Created attachment 262007 [details]
Using LayoutUnit instead of plain double types.

[Javier Fernandez]

Created attachment 262063 [details]
Patch

[Javier Fernandez]

Created attachment 262066 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262066 [details]
Patch

Clearing flags on attachment: 262066

Committed r190308: <http://trac.webkit.org/changeset/190308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.