WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 139584
[details]
Patch 4
Build Bot
Comment 21
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
yosin
Comment 22
2012-04-30 23:10:53 PDT
Created
attachment 139585
[details]
Patch 5
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
Created
attachment 143753
[details]
Patch 9
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
Comment on
attachment 145264
[details]
Patch 10
Attachment 145264
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/12875043
Build Bot
Comment 46
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
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.
Top of Page
Format For Printing
XML
Clone This Bug