WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
148944
[CSS Grid Layout] Flex tracks sizing alg must handle 0fr values
https://bugs.webkit.org/show_bug.cgi?id=148944
Summary
[CSS Grid Layout] Flex tracks sizing alg must handle 0fr values
Javier Fernandez
Reported
2015-09-07 14:00:40 PDT
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.
Attachments
Patch
(26.31 KB, patch)
2015-09-09 15:48 PDT
,
Javier Fernandez
no flags
Details
Formatted Diff
Diff
Patch
(27.52 KB, patch)
2015-09-23 13:07 PDT
,
Javier Fernandez
no flags
Details
Formatted Diff
Diff
Using LayoutUnit instead of plain double types.
(27.51 KB, patch)
2015-09-28 05:23 PDT
,
Javier Fernandez
no flags
Details
Formatted Diff
Diff
Patch
(27.43 KB, patch)
2015-09-29 04:23 PDT
,
Javier Fernandez
no flags
Details
Formatted Diff
Diff
Patch
(27.45 KB, patch)
2015-09-29 04:36 PDT
,
Javier Fernandez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Javier Fernandez
Comment 1
2015-09-09 15:48:15 PDT
Created
attachment 260884
[details]
Patch
Darin Adler
Comment 2
2015-09-09 16:17:26 PDT
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 3
2015-09-11 16:59:06 PDT
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 4
2015-09-18 06:34:43 PDT
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
Comment 5
2015-09-23 13:07:09 PDT
Created
attachment 261838
[details]
Patch
Javier Fernandez
Comment 6
2015-09-23 13:11:07 PDT
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
Comment 7
2015-09-28 05:23:06 PDT
Created
attachment 262007
[details]
Using LayoutUnit instead of plain double types.
Javier Fernandez
Comment 8
2015-09-29 04:23:49 PDT
Created
attachment 262063
[details]
Patch
Javier Fernandez
Comment 9
2015-09-29 04:36:41 PDT
Created
attachment 262066
[details]
Patch
WebKit Commit Bot
Comment 10
2015-09-29 05:33:12 PDT
Comment on
attachment 262066
[details]
Patch Clearing flags on attachment: 262066 Committed
r190308
: <
http://trac.webkit.org/changeset/190308
>
WebKit Commit Bot
Comment 11
2015-09-29 05:33:17 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug