RESOLVED FIXED Bug 80009
[Forms] Introduce Decimal arithmetic to fix rounding errors in number/range input types
https://bugs.webkit.org/show_bug.cgi?id=80009
Summary [Forms] Introduce Decimal arithmetic to fix rounding errors in number/range i...
Kent Tamura
Reported 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"
Attachments
Patch 1 - for Preliminary Review (191.30 KB, patch)
2012-04-12 03:38 PDT, yosin
no flags
Patch 2 - for Preliminary Review (107.04 KB, patch)
2012-04-15 19:23 PDT, yosin
no flags
Patch 3 - for Preliminary Review (80.36 KB, patch)
2012-04-27 04:44 PDT, yosin
no flags
Patch 4 (86.03 KB, patch)
2012-04-30 22:44 PDT, yosin
no flags
Patch 5 (86.22 KB, patch)
2012-04-30 23:10 PDT, yosin
no flags
Patch 6 - For Makefile validation (151.34 KB, patch)
2012-05-23 03:51 PDT, yosin
no flags
Patch 7 - For Makefile validation (146.24 KB, patch)
2012-05-23 20:27 PDT, yosin
no flags
Patch 8 - For Makefile validation (146.27 KB, patch)
2012-05-23 21:02 PDT, yosin
no flags
Patch 9 (197.72 KB, patch)
2012-05-24 00:48 PDT, yosin
no flags
Patch 10 (126.52 KB, patch)
2012-06-01 03:49 PDT, yosin
no flags
Patch 11 (126.13 KB, patch)
2012-06-03 18:18 PDT, yosin
tkent: review-
Sameer Patil
Comment 1 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.
Kent Tamura
Comment 2 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));
yosin
Comment 3 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
Kent Tamura
Comment 4 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.
Sameer Patil
Comment 5 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
yosin
Comment 6 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.
yosin
Comment 7 2012-04-12 03:38:29 PDT
Created attachment 136870 [details] Patch 1 - for Preliminary Review
WebKit Review Bot
Comment 8 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.
Kent Tamura
Comment 9 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
Kent Tamura
Comment 10 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.
yosin
Comment 11 2012-04-15 19:23:50 PDT
Created attachment 137264 [details] Patch 2 - for Preliminary Review
Kent Tamura
Comment 12 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
yosin
Comment 13 2012-04-27 04:44:37 PDT
Created attachment 139169 [details] Patch 3 - for Preliminary Review
Early Warning System Bot
Comment 14 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
Build Bot
Comment 15 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
Kent Tamura
Comment 16 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.
yosin
Comment 17 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.
yosin
Comment 18 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.
yosin
Comment 19 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.
yosin
Comment 20 2012-04-30 22:44:17 PDT
Build Bot
Comment 21 2012-04-30 23:02:43 PDT
yosin
Comment 22 2012-04-30 23:10:53 PDT
Kent Tamura
Comment 23 2012-05-01 23:07:48 PDT
Let's change the summary to match to what we do.
Kent Tamura
Comment 24 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.
Kent Tamura
Comment 25 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).
Kent Tamura
Comment 26 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.
yosin
Comment 27 2012-05-23 03:51:52 PDT
Created attachment 143523 [details] Patch 6 - For Makefile validation
Early Warning System Bot
Comment 28 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
Early Warning System Bot
Comment 29 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
Build Bot
Comment 30 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
WebKit Review Bot
Comment 31 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
Build Bot
Comment 32 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
yosin
Comment 33 2012-05-23 20:27:16 PDT
Created attachment 143716 [details] Patch 7 - For Makefile validation
Build Bot
Comment 34 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
Early Warning System Bot
Comment 35 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
Early Warning System Bot
Comment 36 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
yosin
Comment 37 2012-05-23 21:02:53 PDT
Created attachment 143719 [details] Patch 8 - For Makefile validation
WebKit Review Bot
Comment 38 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
yosin
Comment 39 2012-05-24 00:48:50 PDT
yosin
Comment 40 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.
Kent Tamura
Comment 41 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.
Nikolas Zimmermann
Comment 42 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.
yosin
Comment 43 2012-06-01 03:49:04 PDT
Created attachment 145264 [details] Patch 10
Early Warning System Bot
Comment 44 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
Early Warning System Bot
Comment 45 2012-06-01 04:21:16 PDT
Build Bot
Comment 46 2012-06-01 04:27:11 PDT
WebKit Review Bot
Comment 47 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
yosin
Comment 48 2012-06-03 18:18:37 PDT
Created attachment 145498 [details] Patch 11
yosin
Comment 49 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.
Kent Tamura
Comment 50 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.
Kent Tamura
Comment 51 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.
yosin
Comment 52 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.
yosin
Comment 53 2012-06-17 23:04:02 PDT
This bug is also reported as http://crbug.com/125500
Note You need to log in before you can comment on or make changes to this bug.