Bug 16652 - Firefox and JavaScriptCore differ in Number.toString(integer)
Summary: Firefox and JavaScriptCore differ in Number.toString(integer)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://trac.webkit.org/projects/webki...
Keywords:
Depends on: 16672 63886
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-28 20:14 PST by Eric Seidel (no email)
Modified: 2011-07-04 19:59 PDT (History)
2 users (show)

See Also:


Attachments
Preliminary patch, requires testing & new layout test results. (28.79 KB, patch)
2011-06-30 23:25 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
The patch (48.51 KB, patch)
2011-07-01 11:28 PDT, Gavin Barraclough
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
The patch, style fixes (49.06 KB, patch)
2011-07-01 11:38 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix Qt bot's style quibbles. (49.09 KB, patch)
2011-07-01 11:41 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff
Patch with build fixes (58.88 KB, patch)
2011-07-04 19:46 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-12-28 20:14:07 PST
Firefox and JavaScriptCore differ in Number.toString(integer)

See:
http://trac.webkit.org/projects/webkit/browser/trunk/LayoutTests/fast/js/number-toString-expected.txt?rev=29020

Opera agrees with us.  We'll need to check IE.  If IE agrees with us, then we should check in passing results and file a bug with Mozilla.
Comment 1 Eric Seidel (no email) 2007-12-29 19:32:29 PST
Unfortunately the spec isn't very helpful here:
http://bclary.com/2004/11/07/#a-15.7.4.2

So we simply chose to match Firefox (or IE if we determine their behavior to be different).
Comment 2 David Kilzer (:ddkilzer) 2007-12-29 22:25:20 PST
(In reply to comment #1)
> So we simply chose to match Firefox (or IE if we determine their behavior to be
> different).

Results from running MSIE 7 on Windows XP SP2 in Virtual PC 7.0.3 on Mac OS X 10.4.11 (8S165):

PASS (0.0).toString(4) is "0"
PASS (-0.0).toString(4) is "0"
PASS (0.0).toString() is "0"
PASS (-0.0).toString() is "0"
PASS (1234.567).toString() is "1234.567"
PASS (1234.567).toString(0) threw exception [object Error].
PASS (1234.567).toString(null) threw exception [object Error].
PASS (1234.567).toString(false) threw exception [object Error].
PASS (1234.567).toString('foo') threw exception [object Error].
PASS (1234.567).toString(nan) threw exception [object Error].
PASS (1234.567).toString(1) threw exception [object Error].
PASS (1234.567).toString(true) threw exception [object Error].
PASS (1234.567).toString('1') threw exception [object Error].
FAIL (1234.567).toString(3) should be 1200201.120022100021001021021002202. Was 1200201.120022100021001021021002201.
PASS (1234.567).toString(4) is "103102.21010212322113203111"
FAIL (1234.567).toString(5) should be 14414.240414141414141414. Was 14414.2404141414141414142.
FAIL (1234.567).toString(6) should be 5414.32224554134430233. Was 5414.322245541344302332.
FAIL (1234.567).toString(7) should be 3412.365323661111653. Was 3412.3653236611116525.
PASS (1234.567).toString(8) is "2322.44223351361524"
PASS (1234.567).toString(9) is "1621.50830703723265"
PASS (1234.567).toString(10) is "1234.567"
FAIL (1234.567).toString(11) should be a22.62674a0a5885. Was a22.62674a0a588491.
FAIL (1234.567).toString(12) should be 86a.697938b17701. Was 86a.697938b177012.
FAIL (1234.567).toString(13) should be 73c.74a91191a65. Was 73c.74a91191a64cb.
PASS (1234.567).toString(14) is "642.7d1bc2caa757"
FAIL (1234.567).toString(15) should be 574.87895959596. Was 574.87895959595a.
PASS (1234.567).toString(16) is "4d2.9126e978d5"
PASS (1234.567).toString(17) is "44a.9aeb6faa0da"
FAIL (1234.567).toString(18) should be 3ea.a3cd7102ac. Was 3ea.a3cd7102abf.
FAIL (1234.567).toString(19) should be 37i.aed102a04d. Was 37i.aed102a04d4.
FAIL (1234.567).toString(20) should be 31e.b6g. Was 31e.b6g00000001.
PASS (1234.567).toString(21) is "2gg.bj0kf5cfe9"
FAIL (1234.567).toString(22) should be 2c2.ca9937cak. Was 2c2.ca9937cak1.
PASS (1234.567).toString(23) is "27f.d0lfjb1a7c"
FAIL (1234.567).toString(24) should be 23a.dee4nj99j. Was 23a.dee4nj99j0.
FAIL (1234.567).toString(25) should be 1o9.e49999999. Was 1o9.e49999999a.
FAIL (1234.567).toString(26) should be 1lc.ej7fa4pkf. Was 1lc.ej7fa4pkfb.
FAIL (1234.567).toString(27) should be 1ij.f8971772k. Was 1ij.f8971772j.
PASS (1234.567).toString(28) is "1g2.foelqia8e"
FAIL (1234.567).toString(29) should be 1dg.gcog9e05q. Was 1dg.gcog9e05p.
FAIL (1234.567).toString(30) should be 1b4.h09. Was 1b4.h09000000.
FAIL (1234.567).toString(31) should be 18p.hhrfcj3t. Was 18p.hhrfcj3t1.
PASS (1234.567).toString(32) is "16i.i4jeiu6l"
FAIL (1234.567).toString(33) should be 14d.inf96rdvm. Was 14d.inf96rdvl.
FAIL (1234.567).toString(34) should be 12a.j9fchdtm. Was 12a.j9fchdtm1.
FAIL (1234.567).toString(35) should be 109.jtk4d4d4e. Was 109.jtk4d4d4d.
FAIL (1234.567).toString(36) should be ya.kety9sifl. Was ya.kety9sifkl.
PASS (1234.567).toString(37) threw exception [object Error].
PASS (1234.567).toString(-1) threw exception [object Error].
PASS (1234.567).toString(posInf) threw exception [object Error].
PASS (1234.567).toString(negInf) threw exception [object Error].
PASS posInf.toString() is "Infinity"
PASS negInf.toString() is "-Infinity"
PASS nan.toString() is "NaN"
PASS successfullyParsed is true

TEST COMPLETE
Comment 3 Eric Seidel (no email) 2007-12-29 22:55:59 PST
Since the checked in results match Firefox, it looks like IE and Firefox are nearly identical.  So we're in the wrong here.  I guess we'll eventually fix our Number.toString implementation to match IE & FF.
Comment 4 Eric Seidel (no email) 2007-12-30 00:44:23 PST
I fixed the throwing exceptions issue as part of bug 16672.
Comment 5 Gavin Barraclough 2011-06-30 23:25:27 PDT
Created attachment 99429 [details]
Preliminary patch, requires testing & new layout test results.
Comment 6 Gavin Barraclough 2011-07-01 11:28:16 PDT
Created attachment 99490 [details]
The patch
Comment 7 WebKit Review Bot 2011-07-01 11:31:36 PDT
Attachment 99490 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/NumberPrototype.cpp:118:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:320:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:370:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:526:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:636:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:636:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Early Warning System Bot 2011-07-01 11:37:30 PDT
Comment on attachment 99490 [details]
The patch

Attachment 99490 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8978036
Comment 9 Gavin Barraclough 2011-07-01 11:38:40 PDT
Created attachment 99494 [details]
The patch, style fixes
Comment 10 Gavin Barraclough 2011-07-01 11:41:29 PDT
Created attachment 99495 [details]
Fix Qt bot's style quibbles.
Comment 11 Sam Weinig 2011-07-01 13:47:26 PDT
Comment on attachment 99495 [details]
Fix Qt bot's style quibbles.

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

Overall, this looks really good.  I think we should put the two classes in their own headers in WTF and even write some unit tests for them in TestWebKitAPI.  You should probably also move decomposeDouble to MathExtras.h and again, consider unit tests.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:130
> +// A value 1 greater than max uint16_t.
> +#define Uint16Infinity 0x10000

Please put a comment about why this is necessary to work around a compiler issue.  I also find the name a little confusing.  Perhaps be explicit and call it something like oneGreaterThanMaxUInt16.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:351
> +    bool isNormalized() const

Normalized is a bit odd here, since there are so many different concepts of normal in double-ish math.  Perhaps something lengthy including the word valid would help.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:494
> +    double fractionPart = number - floor(number);

The floor(number) is redundant, you can just use integerPart.
Comment 12 Gavin Barraclough 2011-07-03 15:29:45 PDT
Sam,

I've landed this in r90347 on the basis of your r+ & the fact that the code is covered by tests in fast/js, without unit tests.  The unit testing framework is new to me, and given this code has test coverage it makes sense to me to land this, and follow up with a patch to add additional test cases once I have a chance to sit down with you & learn how to write unit tests.  Hope this sounds right to you.

cheers,
G.
Comment 13 Kent Tamura 2011-07-03 20:50:00 PDT
r90347 caused build erros on Leopard, WinCariro, WinCE, and Chromium-win.

http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/36837/steps/compile-webkit/logs/stdio

/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/JavaScriptCore/runtime/Uint16WithFraction.h: In member function 'JSC::Uint16WithFraction& JSC::Uint16WithFraction::operator*=(uint16_t)':
/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/JavaScriptCore/runtime/Uint16WithFraction.h:125: warning: implicit conversion shortens 64-bit value into a 32-bit value


http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/8133/steps/compile-webkit/logs/stdio

10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(266) : error C2061: syntax error : identifier 'int32_t'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2065: 'uint64_t' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2146: syntax error : missing ';' before identifier 'bits'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2065: 'bits' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2783: 'TO WTF::bitwise_cast(FROM)' : could not deduce template argument for 'TO'
10>        D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/StdLibExtras.h(94) : see declaration of 'WTF::bitwise_cast'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(273) : error C2065: 'exponent' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(273) : error C2061: syntax error : identifier 'int32_t'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(274) : error C2065: 'mantissa' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\Source\JavaScriptCore\os-win32\stdint.h(42) : error C2378: 'uint64_t' : redefinition; symbol cannot be overloaded with a typedef
Comment 14 Kent Tamura 2011-07-03 20:57:05 PDT
Rolled out as r90349.
Comment 15 Gavin Barraclough 2011-07-04 19:46:16 PDT
Created attachment 99666 [details]
Patch with build fixes
Comment 16 Gavin Barraclough 2011-07-04 19:58:29 PDT
Oh.  The last patch didn't cause errors on the EWS bots anyway, so watching them won't help. :-(  LAnding patch & watching the buildbots.
Comment 17 Gavin Barraclough 2011-07-04 19:59:35 PDT
Re-landed in r90383 with build fixes; watching the build bots.