[webkit-reviews] review granted: [Bug 218880] Serialize CSS <number> values with rounding, limited decimal precision, and no exponents per-spec : [Attachment 414235] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 16 12:02:35 PST 2020


Darin Adler <darin at apple.com> has granted Tyler Wilcock
<twilco.o at protonmail.com>'s request for review:
Bug 218880: Serialize CSS <number> values with rounding, limited decimal
precision, and no exponents per-spec
https://bugs.webkit.org/show_bug.cgi?id=218880

Attachment 414235: Patch

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 414235
  --> https://bugs.webkit.org/attachment.cgi?id=414235
Patch

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

I think this is a step in the right direction.

At least some of these are also showing wrong results that seem to be due to
unwanted mixing of float and double (for example, the string "200.7" turning
into "200.699997px", but that's in a test that is supposed to forbid fractional
values at all), but that’s a separate issue. This does seem to match the
specification language, but I’m not sure there’s enough test coverage of edge
cases for this, even in WPT. I’d like to see more test cases.

However, this implementation is subtly wrong, so please use FormattedNumber in
the version you land.

> Source/WTF/wtf/dtoa.cpp:123
> +	   // If we've truncated the trailing zeros and a trailing decimal, we
may have a -0. Remove the negative sign in this case.
> +	   if (builder.position() == 2 && buffer[0] == '-' && buffer[1] == '0')
> +	       builder.RemoveCharacters(0, 1);

What about the case where the value *is* negative zero?

> Source/WTF/wtf/text/WTFString.cpp:510
> +String String::numberToStringFixedWidth(double number, unsigned
decimalPlaces, TrailingZerosTruncatingPolicy trailingZerosTruncatingPolicy)

I think you could use a shorter name for this local, since its scope is really
small and there is no ambiguity. Maybe just "policy" in fact.

> Source/WebCore/css/CSSPrimitiveValue.cpp:909
> +    return makeString(String::numberToStringFixedWidth(m_value.num, 6,
TruncateTrailingZeros), suffix);

Instead of String::numberToStringFixedWidth, this should be using
FormattedNumber::fixedWidth. That is more efficient and avoids allocating
memory every time.

That’s going to mean that instead of (or in addition to) extending WTFString.h,
we will want to extend StringConcatenateNumbers.h to add the truncate trailing
zeroes feature to FormattedNumber::fixedWidth.

>
LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/animation/flex-shri
nk-interpolation-expected.txt:22
> -FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3)
should be [1.65] assert_equals: expected "1.65 " but got "1.55 "
> +FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3)
should be [1.65] assert_equals: expected "1.65 " but got "1.54 "

What explains this change and the others like it, where the second digit after
the decimal point changes?


More information about the webkit-reviews mailing list