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.
Created attachment 260884 [details] Patch
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?
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.
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.
Created attachment 261838 [details] Patch
Even that we had r+ on the previous patch, I've added a new one addressing some of the issues commented in #c2
Created attachment 262007 [details] Using LayoutUnit instead of plain double types.
Created attachment 262063 [details] Patch
Created attachment 262066 [details] Patch
Comment on attachment 262066 [details] Patch Clearing flags on attachment: 262066 Committed r190308: <http://trac.webkit.org/changeset/190308>
All reviewed patches have been landed. Closing bug.