[webkit-reviews] review denied: [Bug 234917] [css-grid] Fix rounding of distributed free space to flexible tracks : [Attachment 448493] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied zsun at igalia.com's request for review:
Bug 234917: [css-grid] Fix rounding of distributed free space to flexible
tracks
https://bugs.webkit.org/show_bug.cgi?id=234917

Attachment 448493: Patch

https://bugs.webkit.org/attachment.cgi?id=448493&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 448493
  --> https://bugs.webkit.org/attachment.cgi?id=448493
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.