Bug 80009 - [Forms] Introduce Decimal arithmetic to fix rounding errors in number/range input types
Summary: [Forms] Introduce Decimal arithmetic to fix rounding errors in number/range i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: yosin
URL: http://jsfiddle.net/8Z5kq/
Keywords: WebExposed
Depends on: 82034 85959 87077 87360 87941 88044 88220 88275 88383 88746
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-02-29 23:35 PST by Kent Tamura
Modified: 2012-06-17 23:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch 1 - for Preliminary Review (191.30 KB, patch)
2012-04-12 03:38 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 - for Preliminary Review (107.04 KB, patch)
2012-04-15 19:23 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 - for Preliminary Review (80.36 KB, patch)
2012-04-27 04:44 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (86.03 KB, patch)
2012-04-30 22:44 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 5 (86.22 KB, patch)
2012-04-30 23:10 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 6 - For Makefile validation (151.34 KB, patch)
2012-05-23 03:51 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 7 - For Makefile validation (146.24 KB, patch)
2012-05-23 20:27 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 8 - For Makefile validation (146.27 KB, patch)
2012-05-23 21:02 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 9 (197.72 KB, patch)
2012-05-24 00:48 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 10 (126.52 KB, patch)
2012-06-01 03:49 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 11 (126.13 KB, patch)
2012-06-03 18:18 PDT, yosin
tkent: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-02-29 23:35:46 PST
Try:
data:text/html,<input type=range step=0.1 value=0.6 ><script>document.write(document.getElementsByTagName('input')[0].value);</script>

It should show '0.6', not '0.6000000000000001'


This was found by IE Test Center:
http://samples.msdn.microsoft.com/ietestcenter/#html5Range
"Set the 'value' to any numeric value in markup"
Comment 1 Sameer Patil 2012-03-16 03:27:59 PDT
This issue looks to be because of wrong step factor representation in double value. Here step factor is "0.1" when it is converted from string to double it is represented as "0.10000000000000001".

Can anyone please confirm whether it is related to representation error mentioned here http://www.network-theory.co.uk/docs/pytut/RepresentationError.html.
Comment 2 Kent Tamura 2012-03-16 03:41:05 PDT
(In reply to comment #1)
> This issue looks to be because of wrong step factor representation in double value. Here step factor is "0.1" when it is converted from string to double it is represented as "0.10000000000000001".
> 
> Can anyone please confirm whether it is related to representation error mentioned here http://www.network-theory.co.uk/docs/pytut/RepresentationError.html.

Yeah, this is caused by the representation error in IEEE754.
We should update RangeInputType::sanitizeValue() like:

  If proposedValue satisfies min/max/step restrictions,
    return proposedValue
  return serializeForNumberType(StepRange(element()).clampValue(proposedValue));
Comment 3 yosin 2012-03-20 23:06:12 PDT
Add another sample, it displays
0
0.1
0.2
0.30000000000000004
0.4
0.5
0.6000000000000001
0.7000000000000001
0.8
0.9

It is better to change double print function to use "%.15f"?

#include <stdio.h>
int main(int, char**) {
   for (int i = 1; i <= 9; i++) {
       double x = i * 0.1;
       printf("%d %f %.15f %.16f %.17f\n", i, x, x, x, x);
   }
   return 0;
}

1 0.100000 0.100000000000000 0.1000000000000000 0.10000000000000001
2 0.200000 0.200000000000000 0.2000000000000000 0.20000000000000001
3 0.300000 0.300000000000000 0.3000000000000000 0.30000000000000004
4 0.400000 0.400000000000000 0.4000000000000000 0.40000000000000002
5 0.500000 0.500000000000000 0.5000000000000000 0.50000000000000000
6 0.600000 0.600000000000000 0.6000000000000001 0.60000000000000009
7 0.700000 0.700000000000000 0.7000000000000001 0.70000000000000007
8 0.800000 0.800000000000000 0.8000000000000000 0.80000000000000004
9 0.900000 0.900000000000000 0.9000000000000000 0.90000000000000002
Comment 4 Kent Tamura 2012-03-20 23:44:20 PDT
(In reply to comment #3)
> It is better to change double print function to use "%.15f"?

I don't think so. 
A simple fix is to avoid resetting a valid value as I wrote in Comment 2.
Another fix is to use HTMLInputElement::alignValueForStep() or something like it.
Comment 5 Sameer Patil 2012-03-21 00:06:45 PDT
> Another fix is to use HTMLInputElement::alignValueForStep() or something like it.
More details on similar issue logged for number input element https://bugs.webkit.org/show_bug.cgi?id=48308
Comment 6 yosin 2012-03-22 00:10:57 PDT
How about using std::decimal (aka ISO C++ TR24733[1]) available GCC 4.5.
decimal128 supports 
  exponent -6143 to 6144
  digits 34

It is wider than double(float64).

[1] http://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.tr24733

Until all compilers support decimal128, we should have third party one or implemented by ourselves, not full version.
Comment 7 yosin 2012-04-12 03:38:29 PDT
Created attachment 136870 [details]
Patch 1 - for Preliminary Review
Comment 8 WebKit Review Bot 2012-04-12 03:42:17 PDT
Attachment 136870 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/Decimal128Test.cpp:97:  uint64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/Decimal128.cpp:127:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128.cpp:129:  Missing spaces around ==  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128.cpp:130:  Missing spaces around !=  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128.cpp:131:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128.cpp:132:  Missing spaces around <=  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128.cpp:134:  Missing spaces around >=  [whitespace/operators] [3]
Source/WebCore/platform/Decimal128Impl.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/Decimal128Impl.cpp:92:  uint64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 9 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Kent Tamura 2012-04-12 06:45:24 PDT
Comment on attachment 136870 [details]
Patch 1 - for Preliminary Review

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

Would you show how many bytes will the WebCore (or a browser) size increase by?

> Source/WebCore/platform/Decimal128.cpp:13
> + * 1.    Redistributions of source code must retain the above copyright
> + *         notice, this list of conditions and the following disclaimer.
> + * 2.    Redistributions in binary form must reproduce the above copyright
> + *         notice, this list of conditions and the following disclaimer in the
> + *         documentation and/or other materials provided with the distribution.
> + *

This is not a copyright header which Google employees usually use.

> Source/WebCore/platform/Decimal128.h:50
> +    Decimal128 operator +(double) const;

I don't think we need operators for all of these types (double, int32_t, int64_t, uint32_t, uint64_t).  Please remove unnecessary operators.

> Source/WebCore/platform/Decimal128Impl.cpp:37
> +namespace {

We don't use anonymous namespace in WebKit.

> Source/WebCore/platform/Decimal128Impl.cpp:71
> +static const int kDpdBits = 10;
> +static const int kDpdMask = (1 << kDpdBits) - 1; // = 0x3FF

We don't use k-prefixes.
We use all-capital or all-lowercase letters for acronyms.
http://www.webkit.org/coding/coding-style.html#names-basic
Comment 10 Kent Tamura 2012-04-12 18:32:00 PDT
Applying decimal arithmetic to solve this issue is probably nice, but we don't need compliance with IEEE754 decimal128 at all.  We need a small code to solve this issue.
Comment 11 yosin 2012-04-15 19:23:50 PDT
Created attachment 137264 [details]
Patch 2 - for Preliminary Review
Comment 12 Kent Tamura 2012-04-15 21:08:02 PDT
Comment on attachment 137264 [details]
Patch 2 - for Preliminary Review

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

Please do not set r? for an incomplete patch.
I hesitate to review this patch until the patch actually resolves the type=number/range issues.


> Source/WebCore/platform/Decimal128Impl.cpp:42
> +namespace {

We don't use anonymous namespace.

> Source/WebCore/platform/Decimal128Impl.cpp:164
> +class BcdNum {

This should be BCDNumber or something.
http://www.webkit.org/coding/coding-style.html#names-basic
http://www.webkit.org/coding/coding-style.html#names-full-words
Comment 13 yosin 2012-04-27 04:44:37 PDT
Created attachment 139169 [details]
Patch 3 - for Preliminary Review
Comment 14 Early Warning System Bot 2012-04-27 04:56:52 PDT
Comment on attachment 139169 [details]
Patch 3 - for Preliminary Review

Attachment 139169 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12554172
Comment 15 Build Bot 2012-04-27 05:13:44 PDT
Comment on attachment 139169 [details]
Patch 3 - for Preliminary Review

Attachment 139169 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12549181
Comment 16 Kent Tamura 2012-04-27 06:38:14 PDT
Comment on attachment 139169 [details]
Patch 3 - for Preliminary Review

(In reply to comment #12)
> Please do not set r? for an incomplete patch.
Comment 17 yosin 2012-04-27 08:41:49 PDT
Comment on attachment 139169 [details]
Patch 3 - for Preliminary Review

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

> Source/WebCore/ChangeLog:1
> +2012-04-27  Yoshifumi Inoue  <yosin@chromium.org>

You should add Decimal.cpp and Decimal.h to WebCore/WebCore.vcproj for Windows build.

> Source/WebCore/html/RangeInputType.cpp:248
> +        setValueAsNumber(parseToDouble(newValue.toString(), numeric_limits<double>::quiet_NaN()), eventBehavior, ec);

You should call element()->setValue rather than converting Decimal to double. RangeInputType::setValueAsNumber == element()->setValue(serialize(value), eventBehavior). Note: It doesn't pass ExcpetionCode to element()->setValue. Because of handleKeydownEvent method doesn't care about exception during setValue.

> Source/WebCore/html/StepRange.cpp:44
> +    maximum = Decimal::fromString(serializeForNumberType(element->maximum()));

StepRange class should retrieve maximum and minimum from HTML attribute and RangeType::maximum and minimum use StepRange class.

> Source/WebCore/html/StepRange.cpp:65
> +    return value.isFinite() ? clampValue(value) : clampValue((minimum + maximum) / 2);

We should use StepRange::defaultValue which is clampValue((minimum + maximum) / 2).

> Source/WebCore/html/StepRange.cpp:74
> +        return clampValue((minimum + maximum) / 2);

We should use StepRange::defaultValue which is clampValue((minimum + maximum) / 2).

> Source/WebCore/platform/Decimal.cpp:49
> +static const uint64_t MaxCoefficient = 0x16345785D89FFFF; // 999999999999999999; // 18 9's

We should add "ull" suffix. The compiler used in Qt build doesn't accept this.

> Source/WebCore/platform/Decimal.cpp:753
> +                }

We should count number of extra digits and add to result exponent.

> Source/WebCore/platform/Decimal.cpp:859
> +            if (ch == '-') {

Leading "+" sign isn't not "valid floating point number" in HTML5 spec (2.5.4.3 Floating-pointer numbers).
However, "rules for parsing floating point number values" allows parse string contains "+" sign.
We'll file a bug about this and change Decimal::fromString and parseToDoubleForNumberType.
Note: WTFString::toDouble (StringToDoubleConverter::StringToDouble) accepts "+".

> Source/WebCore/platform/Decimal.cpp:903
> +            return zero(0);

We should return NaN if result is not (2^-1024, 2^1024).
DBL_MIN_10_EXP=-307
least-positive-normalized-double-float=2.2250738585072014d-308
(integer-decode-float least-positive-normalized-double-float) = significand=4503599627370496(==1<<52), exponent=-1074=-1024-53

> Source/WebCore/platform/Decimal.cpp:913
> +        return Decimal(accumulator, resultExponent, minus);

We should return NaN if result is not (2^-1024, 2^1024).
DBL_MAX=1.7976931348623157e+308

> Source/WebCore/platform/Decimal.cpp:956
> +Decimal Decimal::roundAt(int numberOfDigits)

Please remove unused method Decimal::roundAt.

> Source/WebCore/platform/Decimal.cpp:981
> +        return sign() ? "-Infinity" : "Infinity";

Terms "Infinity" and "NaN" come from IEEE 754 specification and they are also found in http://trac.webkit.org/browser/trunk/Source/WTF/wtf/dtoa/double-conversion.cc

> Source/WebCore/platform/Decimal.h:122
> +    Decimal roundAt(int);

Please remove unused method Decimal::roundAt.
Comment 18 yosin 2012-04-27 08:50:57 PDT
For Decimal::fromString, how about calling parseToDoubleForNumberType to check "valid floating-point number"? If so, we can share validity checking.
Comment 19 yosin 2012-04-27 09:46:04 PDT
Comment on attachment 139169 [details]
Patch 3 - for Preliminary Review

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

> Source/WebCore/platform/Decimal.cpp:116
> +        x /= 10;

We should use multiplication instead of division, or table look up.
Comment 20 yosin 2012-04-30 22:44:17 PDT
Created attachment 139584 [details]
Patch 4
Comment 21 Build Bot 2012-04-30 23:02:43 PDT
Comment on attachment 139584 [details]
Patch 4

Attachment 139584 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12584528
Comment 22 yosin 2012-04-30 23:10:53 PDT
Created attachment 139585 [details]
Patch 5
Comment 23 Kent Tamura 2012-05-01 23:07:48 PDT
Let's change the summary to match to what we do.
Comment 24 Kent Tamura 2012-05-01 23:42:14 PDT
Comment on attachment 139585 [details]
Patch 5

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

Don't you apply Decimal to NumberInputType?

> Source/WebCore/ChangeLog:62
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::getAllowedValueStep):
> +        (WebCore):
> +        * html/HTMLInputElement.h:
> +        (WebCore):
> +        (HTMLInputElement):
> +        * html/InputType.cpp:
> +        (WebCore::InputType::getAllowedValueStep):
> +        (WebCore):
> +        * html/InputType.h:
> +        (WebCore):
> +        (InputType):
> +        * html/RangeInputType.cpp:
> +        (WebCore::RangeInputType::getAllowedValueStep):
> +        (WebCore):
> +        (WebCore::RangeInputType::handleKeydownEvent):
> +        (WebCore::RangeInputType::fallbackValue):
> +        (WebCore::RangeInputType::maximum):
> +        (WebCore::RangeInputType::minimum):
> +        (WebCore::RangeInputType::sanitizeValue):
> +        * html/RangeInputType.h:
> +        (RangeInputType):
> +        * html/StepRange.cpp:
> +        (WebCore::StepRange::StepRange):
> +        (WebCore::StepRange::clampValue):
> +        (WebCore::StepRange::valueFromElement):
> +        * html/StepRange.h:
> +        (StepRange):
> +        (WebCore::StepRange::defaultValue):
> +        (WebCore::StepRange::proportionFromValue):
> +        (WebCore::StepRange::valueFromProportion):
> +        * html/shadow/SliderThumbElement.cpp:
> +        (WebCore::sliderPosition):
> +        (WebCore::RenderSliderThumb::layout):
> +        (WebCore::SliderThumbElement::setPositionFromPoint):

Please write what you changed / why you changed, especially for existing functions/files.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20206
> +				458A2810154AB5FB00004037 /* Decimal.h */,
> +				458A280E154AB5E600004037 /* Decimal.cpp */,
>  				49E912A40EFAC8E6009D0CAF /* animation */,
>  				FD31604012B026A300C1A359 /* audio */,

I recommend inserting new files to sorted positions in order to avoid patch conflict.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27862
>  				78D02BC5154A18DF00B62D05 /* CSSPropertyAnimation.cpp in Sources */,
> +				458A280F154AB5E600004037 /* Decimal.cpp in Sources */,

ditto.

> Source/WebCore/html/RangeInputType.cpp:114
>  double RangeInputType::minimum() const
>  {
> -    return parseToDouble(element()->fastGetAttribute(minAttr), rangeDefaultMinimum);
> +    return parseToDouble(StepRange(element()).minimum.toString(), numeric_limits<double>::quiet_NaN());
>  }
>  
>  double RangeInputType::maximum() const

Do we need these functions?

> Source/WebCore/html/RangeInputType.cpp:202
> +    const Decimal bigStep = ::std::max((stepRange.maximum - stepRange.minimum) / 10, step);

::std::max() should be max().

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:76
> +    if (!parseToDoubleForNumberType(string, &doubleResult))
> +        return false;
> +    *result = Decimal::fromString(string);

Parsing twice is not reasonable.

> Source/WebCore/platform/Decimal.cpp:353
> +Decimal::Decimal(uint64_t coefficient, int exponent, int sign)
> +    : m_data(coefficient, exponent, sign)

Passing sign as int is unreasonable.  It should be an enum.

> Source/WebCore/platform/Decimal.h:120
> +    static Decimal quietNaN(int);

Do we need to support negative NaN?

> Source/WebKit/chromium/tests/DecimalTest.cpp:113
> +TEST_F(DecimalTest, AppNumberStepUpStepDownFromRenderer)

What's 'App'?

> Source/WebKit/chromium/tests/DecimalTest.cpp:195
> +}

Please add more fromString tests.  e.g.
fromString(".5")
fromString(" 123 ")
fromString("1,234")
fromString("+3")
fromString("INF")
...

> Source/WebKit/chromium/tests/DecimalTest.cpp:216
> +    const Decimal inf(Decimal::infinity(0));
> +    const Decimal minf(Decimal::infinity(1));
> +    const Decimal nan(Decimal::quietNaN(0));
> +    const Decimal mnan(Decimal::quietNaN(1));
> +    const Decimal ten(10);

They should be:
Infinity
MinusInfinity / NegativeInfinity
NaN
MinusNaN / NegativeNaN
Ten

> Source/WebKit/chromium/tests/DecimalTest.cpp:255
> +}

We should add tests for values with larger/smaller exponent.

> Source/WebKit/chromium/tests/DecimalTest.cpp:329
> +}

We should add tests for values with larger/smaller exponent.
Comment 25 Kent Tamura 2012-05-01 23:51:36 PDT
Comment on attachment 139585 [details]
Patch 5

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

> Source/WebKit/chromium/tests/DecimalTest.cpp:117
> +    EXPECT_EQ(String("1"), stepUp("0", "1", "0.33333333333333333", "0", 3)); // step=1/3

We should have a similar test:
  EXPECT_EQ(String("0.01"), stepUp("0", "0.01", "0.0033333333333333333", "0", 3)); // step=1/300

> Source/WebKit/chromium/tests/DecimalTest.cpp:156
> +}

We should have a test for Decimal(-2147483648).
Comment 26 Kent Tamura 2012-05-02 03:11:27 PDT
Comment on attachment 139585 [details]
Patch 5

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

> Source/WebCore/platform/Decimal.cpp:48
> +static const int PrecisionOfExponentBits = 11;
> +static const int PrecisionOfCoefficientBits = 64;

These are not used.

> Source/WebCore/platform/Decimal.cpp:49
> +static const uint64_t MaxCoefficient = 0x16345785D89FFFFull; // 999999999999999999; // 18 9's

I'm afraid this is not compilable with Visual Studio 2008.
See the discussion in Bug 64522.
Comment 27 yosin 2012-05-23 03:51:52 PDT
Created attachment 143523 [details]
Patch 6 - For Makefile validation
Comment 28 Early Warning System Bot 2012-05-23 05:53:19 PDT
Comment on attachment 143523 [details]
Patch 6 - For Makefile validation

Attachment 143523 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12773193
Comment 29 Early Warning System Bot 2012-05-23 05:58:20 PDT
Comment on attachment 143523 [details]
Patch 6 - For Makefile validation

Attachment 143523 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12779154
Comment 30 Build Bot 2012-05-23 05:59:07 PDT
Comment on attachment 143523 [details]
Patch 6 - For Makefile validation

Attachment 143523 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12767262
Comment 31 WebKit Review Bot 2012-05-23 06:03:54 PDT
Comment on attachment 143523 [details]
Patch 6 - For Makefile validation

Attachment 143523 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12780173
Comment 32 Build Bot 2012-05-23 06:07:57 PDT
Comment on attachment 143523 [details]
Patch 6 - For Makefile validation

Attachment 143523 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12778177
Comment 33 yosin 2012-05-23 20:27:16 PDT
Created attachment 143716 [details]
Patch 7 - For Makefile validation
Comment 34 Build Bot 2012-05-23 20:35:54 PDT
Comment on attachment 143716 [details]
Patch 7 - For Makefile validation

Attachment 143716 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12804017
Comment 35 Early Warning System Bot 2012-05-23 20:45:16 PDT
Comment on attachment 143716 [details]
Patch 7 - For Makefile validation

Attachment 143716 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12805015
Comment 36 Early Warning System Bot 2012-05-23 20:48:53 PDT
Comment on attachment 143716 [details]
Patch 7 - For Makefile validation

Attachment 143716 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12791082
Comment 37 yosin 2012-05-23 21:02:53 PDT
Created attachment 143719 [details]
Patch 8 - For Makefile validation
Comment 38 WebKit Review Bot 2012-05-23 21:18:16 PDT
Comment on attachment 143719 [details]
Patch 8 - For Makefile validation

Attachment 143719 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12798055
Comment 39 yosin 2012-05-24 00:48:50 PDT
Created attachment 143753 [details]
Patch 9
Comment 40 yosin 2012-05-24 01:37:18 PDT
Comment on attachment 143753 [details]
Patch 9

* Build with CR-Linux and CR-Mac
* Apply Decimal to Date/DateTime/DateTimeLocal/Month/Time/Week in addition to Number/Range.

Could you review again?
Thanks in advance.
Comment 41 Kent Tamura 2012-05-24 02:17:32 PDT
Comment on attachment 143753 [details]
Patch 9

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

I agree with this strategy, but the patch is too large. 197KB code change in one patch is not acceptable.

Please make another patch with only Decimal.h, Decimal.cpp, and DecimalTest.cpp as the first step.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:92
> +    // FIXME: UINT64_C(17976931348623159) should be used but it did not compile on Qt bots.
> +#if COMPILER(MSVC)
> +    const uint64_t leadingDigitsOf2Power1024 = 17976931348623159ui64;
> +#else
> +    const uint64_t leadingDigitsOf2Power1024 = 17976931348623159ull;
> +#endif

You use this workaround many times.  We should fix this issue before this patch.
I think we can add UINT64_C in wtf/MathExtras.h.

> LayoutTests/fast/forms/range/range-value-rounding-expected.txt:119
> +PASS createSample("0.000001", "0.000009", "1e-5").valueAsNumber is 0.000009
> +FAIL createSample("0.0000001", "0.0000001", "1e-6").value should be 0.0000001. Was 1e-7.
> +PASS createSample("0.0000001", "0.0000001", "1e-6").valueAsNumber is 0.0000001
> +FAIL createSample("0.0000001", "0.0000002", "1e-6").value should be 0.0000002. Was 2e-7.
> +PASS createSample("0.0000001", "0.0000002", "1e-6").valueAsNumber is 0.0000002
> +FAIL createSample("0.0000001", "0.0000003", "1e-6").value should be 0.0000003. Was 3e-7.

This has a lot of FAIL lines.
Comment 42 Nikolas Zimmermann 2012-05-31 03:33:23 PDT
(In reply to comment #6)
> Until all compilers support decimal128, we should have third party one or implemented by ourselves, not full version.

Why don't you just ensure that 0.6000000000001 shows up as 0.6 instead of adding a whole new Decimal class, just to avoid some sanitizing? I'm not sure I follow your strategy here.
Comment 43 yosin 2012-06-01 03:49:04 PDT
Created attachment 145264 [details]
Patch 10
Comment 44 Early Warning System Bot 2012-06-01 03:55:28 PDT
Comment on attachment 145264 [details]
Patch 10

Attachment 145264 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12873237
Comment 45 Early Warning System Bot 2012-06-01 04:21:16 PDT
Comment on attachment 145264 [details]
Patch 10

Attachment 145264 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12875043
Comment 46 Build Bot 2012-06-01 04:27:11 PDT
Comment on attachment 145264 [details]
Patch 10

Attachment 145264 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12863494
Comment 47 WebKit Review Bot 2012-06-01 08:16:33 PDT
Comment on attachment 145264 [details]
Patch 10

Attachment 145264 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12863555
Comment 48 yosin 2012-06-03 18:18:37 PDT
Created attachment 145498 [details]
Patch 11
Comment 49 yosin 2012-06-03 22:11:50 PDT
Comment on attachment 145498 [details]
Patch 11

Could you review this patch?

Note: This patch will be landed after bug 88208 (can't enter "0" to number input type) with rebase.
Comment 50 Kent Tamura 2012-06-04 02:00:01 PDT
Comment on attachment 145498 [details]
Patch 11

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

> Source/WebCore/ChangeLog:1
> +2012-06-03  Yoshifumi Inoue  <yosin@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.
Comment 51 Kent Tamura 2012-06-04 02:52:57 PDT
(In reply to comment #42)
> (In reply to comment #6)
> > Until all compilers support decimal128, we should have third party one or implemented by ourselves, not full version.
> 
> Why don't you just ensure that 0.6000000000001 shows up as 0.6 instead of adding a whole new Decimal class, just to avoid some sanitizing? I'm not sure I follow your strategy here.

The current code is trying to resolve such issues by rounding and passing 'decimalPlaces' information. But
 - We need complex code to keep track decimalPlaces correctly, and one can easily forget to apply decimalPlaces.
 - decimalPlaces is not perfect. We still have bugs in the code with decimalPlaces.
 - The current code needs some precision loss because of rounding and decimalPlaces. We need to support double precision according to the standard.

If we apply decimal arithmetic, the implementation would be really simple.
Comment 52 yosin 2012-06-14 18:40:21 PDT
All blocking patches has been landed.

* Behavior change was started at r119948 (Jun 10 2012 19:33 PST)
* Source code cleanup was done at r120313 (Jun 14 2012, 04:54 PST)

Thanks for waiting about two month with patient.
Comment 53 yosin 2012-06-17 23:04:02 PDT
This bug is also reported as http://crbug.com/125500