Bug 11545

Summary: Disable the old testcases do not follow the ECMA-262v3 specification.
Product: WebKit Reporter: digdog
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: mrowe
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Description Flags
darin: review+
Patch.txt mrowe: review+

Description digdog 2006-11-08 08:01:03 PST
There are 7 tests in js1_2 testcase didn't respect the ECMA-262v3 specification and provided wrong results. And since JavaScriptCore is not going to implement the legacy behavior of old JS versions, I disable these tests.

Comment 1 digdog 2006-11-08 08:01:39 PST
Created attachment 11424 [details]
Comment 2 Maciej Stachowiak 2006-11-10 03:06:21 PST
IT's great to disable the failing tests! However, I suggest adding a comment below each removed test explaining why it is invalid. Also, and more importantly, your new expected results seem to contain new failures. I don't think we want to land that. r- for improper new expected results. It seems to be a date issue, maybe fixed now.
Comment 3 digdog 2006-11-10 06:07:50 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/String/concat.js has 4 tests:

aString.concat([]);  // js1.2 expected: test string[]
aString.concat([1,2,3]);  // js1.2 expected: test string[1,2,3]
'abcde'.concat([]);  // js1.2 expected: abcde[]
'abcde'.concat([1,2,3]); // js1.2 expected: abcde[1,2,3]

According to ECMA, the argument of concat should send to ToString and convert into a string value (not String object). So these arguments will be convert into '' and '1,2,3' under ECMA-262v3, not the js1.2 expected '[]' and '[1,2,3]'

The correct results should be:
test string
test string1,2,3
Comment 4 digdog 2006-11-10 06:22:59 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/function/Number.js has one test:

Number([1,2,3]); // js1.2 expected: 3

According to ECMA 9.3, when input type was Object, should call ToPrimitive(input arg, hint Number) first, and than ToNumber() later. However, ToPrimitive() will use [[DefaultValue]](hint) rule when input Type was Object (ECMA So the input [1,2,3] will applied [[DefaultValue]](hint) rule with hint Number, and it looks like this:

toString(valuOf([1,2,3]))  => toString(1,2,3) => '1,2,3'

Than ToNumber('1,2,3') results NaN based on ECMA 9.3.1: If the grammar cannot interpret the 
string as an expansion of StringNumericLiteral, then the result of ToNumber is NaN.
Comment 5 digdog 2006-11-10 06:36:53 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/function/String.js has 2 tests:

String({p:1}) //js1.2 expected: {p:1}
String([1,2,3]) //js1.2 expect: [1, 2, 3]

According to ECMA 9.8, when input type of String object argument was Object, we should applied ToPrimitive(input arg, hint String) first, and later ToString() . And just like previous one, ToPrimitive() will use [[DefaultValue]](hint) with hint String to convert the input (toString() below uses the rule in ECMA

valueOf(toString({p:1}) => valueOf('[object Object]') => '[object Object]'
valueOf(toString([1,2,3])) => valueOf('1,2,3') => '1,2,3'

And ToString() called after ToPrimitive(), so the correct result would be:

[object Object]
Comment 6 digdog 2006-11-10 06:41:04 PST
"r- for improper new expected results."

Sorry, but what does this mean? I don't get "r-"... 
Comment 7 digdog 2006-11-11 06:19:16 PST
Created attachment 11486 [details]
Comment 8 digdog 2006-11-11 06:19:58 PST
expected result updated.
Comment 9 digdog 2006-11-12 09:11:32 PST
Comment on attachment 11486 [details]

remove patch due non-PST timezone regression appeared
Comment 10 digdog 2006-11-12 09:28:44 PST
Created attachment 11499 [details]

update expected results.
Comment 11 Darin Adler 2006-12-07 15:36:46 PST
Comment on attachment 11499 [details]

This patch looks fine to me, but I'd prefer to see a comment explaining why these tests are disabled instead of just having the tests commented out without explanation.
Comment 12 Darin Adler 2006-12-07 15:39:06 PST
Comment on attachment 11499 [details]

I'll mark this review+, but it would be even better with the comments.
Comment 13 Mark Rowe (bdash) 2006-12-07 16:30:42 PST
digdog, can you please update your patch with the comments that Darin noted so that it can more easily be landed?
Comment 14 digdog 2006-12-07 19:25:02 PST
Created attachment 11769 [details]

Add comments
Comment 15 Mark Rowe (bdash) 2006-12-07 19:34:57 PST
Thanks Kevin!  Landed in r18071, and turned allow-tabs on for the three JS files that were touched as they were already heavily tabified.