[webkit-reviews] review denied: [Bug 80009] [Forms] Introduce Decimal arithmetic to fix rounding errors in number/range input types : [Attachment 145498] Patch 11

Mon Jun 4 02:00:01 PDT 2012

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 80009: [Forms] Introduce Decimal arithmetic to fix rounding errors in
number/range input types
https://bugs.webkit.org/show_bug.cgi?id=80009

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

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

> Source/WebCore/ChangeLog:1
> +2012-06-03  Yoshifumi Inoue	<yosin at chromium.org>

This patch is still too large to be reviewed.
We should have one or more preparation patches.

I'd like to proceed as follows:
 1. Introduce InputNumber type as an alias of double, and replace double with
InputNumber or const InputNumber& in *InputType.{cpp,h}.
 2. Actual behavior change patch.  This makes InputNumber an alias of Decimal.
 3. Replace InputNumber with Decimal.

The next patch would be a preparation of step 1.
 Rename parseToDouble() to parse().
 Rename some doubleValue to numericValue or value
 Replace "static const double" with "static const int" in *InputType.cpp.

After that, we should introduce InputNumber, convertDoubleToInputNumber(),
convertInputNumberToDouble(), and replace function signature to use
InputNumber/const InputNumber&.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:57
> -    return parseToDouble(element()->value(),
DateComponents::invalidMilliseconds());
> +    return convertDecimalToDouble(parseToDecimal(element()->value(),
convertDoubleToDecimal(DateComponents::invalidMilliseconds())));
>  }

I don't think this code works because DateComponents::invalidMilliseconds() is
NaN and Decimal::fromString() doesn't support NaN.

This code is inefficient. How about keeping parseToDouble() as a static
function?

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:184
>      if (!isfinite(parsedValue))
>	   return visibleValue;
>  
> -    return serializeWithMilliseconds(parsedValue);
> +    const Decimal value = convertDoubleToDecimal(parsedValue);
> +    return value.isFinite() ? serializeWithMilliseconds(value) :
visibleValue;

Do we need to check isFinite() again?

> Source/WebCore/html/parser/HTMLParserIdioms.h:74
> +inline double convertDecimalToDouble(const Decimal& value)
> +{
> +    return parseToDoubleForNumberType(serializeForNumberType(value));
> +}
> +
> +inline Decimal convertDoubleToDecimal(double value)
> +{
> +    return parseToDecimalForNumberType(serializeForNumberType(value));
> +}

They shouldn't be here because they're unrelated to HTML parsing.
We should move them to Decimal.{cpp,h}.  We don't need to use
parseToDoubleForNumberType() and serializeForNumberType(double) to implement
them.

> LayoutTests/fast/forms/range/range-value-rounding-expected.txt:549
> +0.1
> +0.1
> +0.2
> +0.2
> +0.3
> +0.3
> +0.4
> +0.4
> +0.5
> +0.5
> +0.6
> +0.6
> +0.7
> +0.7
> +0.8
> +0.8
> +0.9
> +0.9
> +0.01
> +0.01
> +0.02
> +0.02
> +0.03
> +0.03
> +0.04
> +0.04
> +0.05
> +0.05
> +0.06
> +0.06
> +0.07
> +0.07
> +0.08
> +0.08
> +0.09
> +0.09
> +0.001
> +0.001
> +0.002
> +0.002
> +0.003
> +0.003
> +0.004
> +0.004
> +0.005
> +0.005
> +0.006
> +0.006
> +0.007
> +0.007
> +0.008
> +0.008
> +0.009
> +0.009
> +0.0001
> +0.0001
> +0.0002
> +0.0002
> +0.0003
> +0.0003
> +0.0004
> +0.0004
> +0.0005
> +0.0005
> +0.0006
> +0.0006
> +0.0007
> +0.0007
> +0.0008
> +0.0008
> +0.0009
> +0.0009
> +0.00001
> +0.00001
> +0.00002
> +0.00002
> +0.00003
> +0.00003
> +0.00004
> +0.00004
> +0.00005
> +0.00005
> +0.00006
> +0.00006
> +0.00007
> +0.00007
> +0.00008
> +0.00008
> +0.00009
> +0.00009
> +0.000001
> +0.000001
> +0.000002
> +0.000002
> +0.000003
> +0.000003
> +0.000004
> +0.000004
> +0.000005
> +0.000005
> +0.000006
> +0.000006
> +0.000007
> +0.000007
> +0.000008
> +0.000008
> +0.000009
> +0.000009
> +0.0000001
> +0.0000001
> +0.0000002
> +0.0000002
> +0.0000003
> +0.0000003
> +0.0000004
> +0.0000004
> +0.0000005
> +0.0000005
> +0.0000006
> +0.0000006
> +0.0000007
> +0.0000007
> +0.0000008
> +0.0000008
> +0.0000009
> +0.0000009
> +0.00000001
> +0.00000001
> +0.00000002
> +0.00000002
> +0.00000003
> +0.00000003
> +0.00000004
> +0.00000004
> +0.00000005
> +0.00000005
> +0.00000006
> +0.00000006
> +0.00000007
> +0.00000007
> +0.00000008
> +0.00000008
> +0.00000009
> +0.00000009
> +0.000000001
> +0.000000001
> +0.000000002
> +0.000000002
> +0.000000003
> +0.000000003
> +0.000000004
> +0.000000004
> +0.000000005
> +0.000000005
> +0.000000006
> +0.000000006
> +0.000000007
> +0.000000007
> +0.000000008
> +0.000000008
> +0.000000009
> +0.000000009
> +0.0000000001
> +0.0000000001
> +0.0000000002
> +0.0000000002
> +0.0000000003
> +0.0000000003
> +0.0000000004
> +0.0000000004
> +0.0000000005
> +0.0000000005
> +0.0000000006
> +0.0000000006
> +0.0000000007
> +0.0000000007
> +0.0000000008
> +0.0000000008
> +0.0000000009
> +0.0000000009
> +0.00000000001
> +0.00000000001
> +0.00000000002
> +0.00000000002
> +0.00000000003
> +0.00000000003
> +0.00000000004
> +0.00000000004
> +0.00000000005
> +0.00000000005
> +0.00000000006
> +0.00000000006
> +0.00000000007
> +0.00000000007
> +0.00000000008
> +0.00000000008
> +0.00000000009
> +0.00000000009
> +0.000000000001
> +0.000000000001
> +0.000000000002
> +0.000000000002
> +0.000000000003
> +0.000000000003
> +0.000000000004
> +0.000000000004
> +0.000000000005
> +0.000000000005
> +0.000000000006
> +0.000000000006
> +0.000000000007
> +0.000000000007
> +0.000000000008
> +0.000000000008
> +0.000000000009
> +0.000000000009
> +0.0000000000001
> +0.0000000000001
> +0.0000000000002
> +0.0000000000002
> +0.0000000000003
> +0.0000000000003
> +0.0000000000004
> +0.0000000000004
> +0.0000000000005
> +0.0000000000005
> +0.0000000000006
> +0.0000000000006
> +0.0000000000007
> +0.0000000000007
> +0.0000000000008
> +0.0000000000008
> +0.0000000000009
> +0.0000000000009
> +0.00000000000001
> +0.00000000000001
> +0.00000000000002
> +0.00000000000002
> +0.00000000000003
> +0.00000000000003
> +0.00000000000004
> +0.00000000000004
> +0.00000000000005
> +0.00000000000005
> +0.00000000000006
> +0.00000000000006
> +0.00000000000007
> +0.00000000000007
> +0.00000000000008
> +0.00000000000008
> +0.00000000000009
> +0.00000000000009
> +0.000000000000001
> +0.000000000000001
> +0.000000000000002
> +0.000000000000002
> +0.000000000000003
> +0.000000000000003
> +0.000000000000004
> +0.000000000000004
> +0.000000000000005
> +0.000000000000005
> +0.000000000000006
> +0.000000000000006
> +0.000000000000007
> +0.000000000000007
> +0.000000000000008
> +0.000000000000008
> +0.000000000000009
> +0.000000000000009

We had better remove these useless strings.