[webkit-reviews] review denied: [Bug 48308] Too precise serialization from floating point number to string for "number" input elements : [Attachment 72189] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 48308: Too precise serialization from floating point number to string for
"number" input elements
https://bugs.webkit.org/show_bug.cgi?id=48308

Attachment 72189: Patch
https://bugs.webkit.org/attachment.cgi?id=72189&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72189&action=review

> JavaScriptCore/ChangeLog:5
> +	   Too precise serialization from floating point number to string

Please change the summary as Simon did for the bug entry.
We should mention that this affects only to input element.

> JavaScriptCore/JavaScriptCore.exp:392
> +__ZN3WTF14numberToStringEdPtj

We need similar change to
JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def. The
mangled symbol name in VC++ is different from gcc's.

> LayoutTests/fast/forms/script-tests/input-valueasnumber-number.js:24
> +shouldBe('valueAsNumberFor("123456789012345678901234567890123456789")',
'1.23456789012345678E+38');

Why the result is changed? This test doesn't use serializeForNumberType().

> WebCore/html/HTMLInputElement.cpp:302
> +    double acceptableError = step / pow(2.0, FLT_MANT_DIG);

This is specific to type=number.  Other types such as type=date may have
different acceptableError value.
Please introduce InputType::acceptableError(step) and override it in
NumberInputType.  You can merge the same expression in
NumberImputType::stepMismatch().

> WebCore/html/HTMLInputElement.cpp:303
> +    if (newValue - m_inputType->minimum() < -acceptableError) {

Is this change needed to pass existing tests?  If not, we need new test cases.

> WebCore/html/HTMLInputElement.cpp:309
> +    if (newValue - m_inputType->maximum() > acceptableError) {

ditto.