The number of flexible tracks in a grid unevenly divides its leftover space, which causes truncated track sizes not being added as part of the grid available size. Affected WPT test - imported/w3c/web-platform-tests/css/css-grid/layout-algorithm/flex-tracks-with-fractional-size.html
Created attachment 448493 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment on attachment 448493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448493&action=review Looks good. The test is failing on GTK-WK2, which needs to be resolved before landing this. And there are some coding style issues. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:733 > + double leftOverSize = 0; Why double instead of float? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:739 > + const double frShare = flexFraction * trackSize.maxTrackBreadth().flex() + leftOverSize; Why do we compute a double here, and then convert it back to a float? The use of const here isn’t so great. All the local variables could be marked const, like trackIndex, oldBaseSize, and newBaseSize, but normally we don’t do that. Maybe this is Chromium coding style creeping into WebKit? > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:741 > + const LayoutUnit stretchedSize = LayoutUnit(frShare + std::numeric_limits<float>::epsilon()); Same thought on the use of const. I also would suggest writing auto instead of writing LayoutUnit twice on this line. The real goal here is to round rather than truncate, I think. Adding epsilon to do that is a risky hack, sure would be nice to find a better strategy. For example, if we had a LayoutUnit function that converted a float to a LayoutUnit using rounding instead of truncation (just add a call to std::round after multiplying by kFixedPointDenominator) that would be a better solution, and I think it would work. The rest of the patch could likely be the same. There are problems with using epsilon if we ever get to larger exponents, so typically we would use std::nextafter instead of epsilon as a basic coding style technique, but it would be better to use neither. > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:746 > + leftOverSize = std::max(frShare - stretchedSize.toDouble(), 0.0); Again, why double instead of float? Mixing double and float is not great.
Created attachment 448739 [details] Patch
(In reply to Darin Adler from comment #3) > Comment on attachment 448493 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448493&action=review > > Looks good. > > The test is failing on GTK-WK2, which needs to be resolved before landing > this. > Thanks very much for the review comments! Failing in GTK seems related to the calls of "reftest-wait" in the test script. I have raised bug 235025. This might need a separate investigation. > And there are some coding style issues. > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:733 > > + double leftOverSize = 0; > > Why double instead of float? It is a double because flexfraction & flexFraction * trackSize.maxTrackBreadth().flex() are all doubles, right? > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:739 > > + const double frShare = flexFraction * trackSize.maxTrackBreadth().flex() + leftOverSize; > > Why do we compute a double here, and then convert it back to a float? > I thought they are doubles all the way through. Maybe I have missed something here? > The use of const here isn’t so great. All the local variables could be > marked const, like trackIndex, oldBaseSize, and newBaseSize, but normally we > don’t do that. Maybe this is Chromium coding style creeping into WebKit? > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:741 > > + const LayoutUnit stretchedSize = LayoutUnit(frShare + std::numeric_limits<float>::epsilon()); > > Same thought on the use of const. I also would suggest writing auto instead > of writing LayoutUnit twice on this line. > Updated. > The real goal here is to round rather than truncate, I think. Adding epsilon > to do that is a risky hack, sure would be nice to find a better strategy. > For example, if we had a LayoutUnit function that converted a float to a > LayoutUnit using rounding instead of truncation (just add a call to > std::round after multiplying by kFixedPointDenominator) that would be a > better solution, and I think it would work. The rest of the patch could > likely be the same. > I tried using std::round() in LayoutUnit.h, the overall leftovers seems exceeding what the test expected. The width of the image ends up with 106px rather than 100px. On the other thought, I feel that we might not need calling epsilon after all. The purpose of this patch is the recover the fractional part of the computation lost when we floor the stretched size to fit in a LayoutUnit. The leftover itself does this purpose, right? > There are problems with using epsilon if we ever get to larger exponents, so > typically we would use std::nextafter instead of epsilon as a basic coding > style technique, but it would be better to use neither. > > > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:746 > > + leftOverSize = std::max(frShare - stretchedSize.toDouble(), 0.0); > > Again, why double instead of float? Mixing double and float is not great.
Comment on attachment 448493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448493&action=review >>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:739 >>> + const double frShare = flexFraction * trackSize.maxTrackBreadth().flex() + leftOverSize; >> >> Why do we compute a double here, and then convert it back to a float? >> >> The use of const here isn’t so great. All the local variables could be marked const, like trackIndex, oldBaseSize, and newBaseSize, but normally we don’t do that. Maybe this is Chromium coding style creeping into WebKit? > > I thought they are doubles all the way through. Maybe I have missed something here? I think it was inaccurate to say "converted to float"; instead we add the "float epsilon", which itself is converted to a double. That is quite peculiar! I will look at the new patch to see what you chose to do.
Comment on attachment 448739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448739&action=review > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:740 > + auto stretchedSize = LayoutUnit(frShare); This truncates rather than rounding, so it rounds toward zero. Did we want to round in the more traditional way instead? We would do this with the equivalent of LayoutUnit::fromFloatRound, although I suppose we would want a double version of that? Looks like the problem is fixed without that, though, so it may be optional.
(In reply to zsun from comment #5) > I tried using std::round() in LayoutUnit.h, the overall leftovers seems > exceeding what the test expected. The width of the image ends up with 106px > rather than 100px. LayoutUnit::round() rounds a LayoutUnit to a integer. We want to round to the nearest LayoutUnit value (1/64 of an integer), and we want to do that in the process of *creating* the LayoutUnit from a floating point value. Once the LayoutUnit is created the thing we wanted to round is already truncated. > On the other thought, I feel that we might not need calling epsilon after > all. The purpose of this patch is the recover the fractional part of the > computation lost when we floor the stretched size to fit in a LayoutUnit. > The leftover itself does this purpose, right? Sure, that sounds fine. I am not sure about the importance of rounding.
<rdar://problem/87544943>
Committed r287977 (246006@main): <https://commits.webkit.org/246006@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448739 [details].