[Webkit-unassigned] [Bug 218880] Serialize CSS <number> values with rounding, limited decimal precision, and no exponents per-spec

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=218880

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #414235|review?                     |review+, commit-queue-
              Flags|                            |

--- 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-shrink-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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201116/4c87e87b/attachment.htm>