Change the rules for parsing floating-point number values to use finite IEEE 754 double-precision floating-point values

[Kwang Yul Seo]

Remove the following FIXME comment:

// FIXME: We should use numeric_limits<double>::max for number input type.

Use std::numeric_limits<double>::max() instead of std::numeric_limits<float>::max().

[Kwang Yul Seo]

Created attachment 154621 [details]
Patch

[Kwang Yul Seo]

I am quite curious why there is a FIXME comment like this. The FIXME author could change float to double in a second. Any profound reason?

(In reply to comment #2)
> I am quite curious why there is a FIXME comment like this. The FIXME author could change float to double in a second. Any profound reason?

Hi Kwang, thanks for picking up this FIXME.
The change is simple, but we need to update and add test cases.

To fix this FIXME, we would like to do:
 1. Update code like what you did and 
   - Change variable name to "maximum", "maximumNumber" or something unrelated to type.
   - Update comments at line 107. HTML5 spec. uses IEEE 754 double-precision floating point values.
 2. Run layout tests, especially for fast/forms/number, fast/forms/range and update them.
   - I expect some tests will be failed.
 3. Add test cases for std::numeric_limits<double>::max() into fast/forms/number and fast/forms/range
   - number-validity-state-range-overflow and number-validity-state-range-underflow
   - number-stepup-stepdown-from-renderrer, range-stepup-stepdown-from-renderrer
 4. We also need to fix float-max related FIXME in 
    - WebCore/html/NumberInputType.cpp 
    - WebCore/html/StepRange.cpp
  to make layout tests work.
 5. Make sure we have tests for maximum value and minimum value
   <input id=test type=number>
   $('test').valueAsNumber = 1.7976931348623157e308
   shouldBeEqualToString('$("test").value', "1.7976931348623157e308")
   $('test').value = "1.7976931348623157e308"
   shouldEvaluateTo('$("test").valueAsNumber', 1.7976931348623157e308)
   and so on.

[Kwang Yul Seo]

(In reply to comment #3)
Wow. Thanks for explaining this to me. I will work on this!

[Kwang Yul Seo]

Created attachment 155107 [details]
WIP patch

[Kwang Yul Seo]

Comment on attachment 155107 [details]
WIP patch

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

This is my work-in-progress patch. I will add more layout tests.

> Source/WebCore/html/StepRange.cpp:170
>      // Accepts erros in lower fractional part which IEEE 754 single-precision
>      // can't represent.
> -    const Decimal computedAcceptableError = acceptableError();
> +    const Decimal computedAcceptableError = acceptableSinglePrecisionError();

I am not sure what's the right behavior here. What does the HTML5 spec say here?

Comment on attachment 155107 [details]
WIP patch

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

> Source/WebCore/html/NumberInputType.cpp:115
> +    if (newValue < -maximumNumber) {

This condition is true only if newValue is negative infinity.
Do we really need this check? It seems call sites check finite or not.

> Source/WebCore/html/NumberInputType.cpp:-120
> -    if (newValue > floatMax) {

This condition is true only if newValue is positive infinity.
Do we really need this check? It seems call sites check finite or not.

>> Source/WebCore/html/StepRange.cpp:170
>> +    const Decimal computedAcceptableError = acceptableSinglePrecisionError();
> 
> I am not sure what's the right behavior here. What does the HTML5 spec say here?

I think using acceptableError is OK here if acceptableError calculate step / (1 << DBL_MANT_DIG)

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:140
>      // Numbers are considered finite IEEE 754 single-precision floating point values.

Please remove this comment. HTML5 spec uses "double-precision floating point value" now
and section number is different from current spec's section number. HTML5 specs is still
changing, it is hard to synchronize section number in the comment and HTML5 spec.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:142
> +    if (-std::numeric_limits<double>::max() > value || value > std::numeric_limits<double>::max())

This condition expression can't be true. Because, value is finite number.
So, we don't need to have this if statement.

[Kent Tamura]

parseToDecimalForNumberType() is used by HTMLProgressElement and HTMLMeterElement too. We need to add test cases for them.

[Ahmad Saleem]

FIXME is gone now - https://github.com/WebKit/WebKit/commit/134163e4e3101c02038c4447672158e4a2fac445

but I am not sure, if anything else is needed here or not?

@rniwa & @aditya - any input?

[Ahmad Saleem]

*** This bug has been marked as a duplicate of bug 249783 ***