RESOLVED FIXED 5409
slice() testcase doesn't pass 
https://bugs.webkit.org/show_bug.cgi?id=5409
Summary slice() testcase doesn't pass
George Staikos
Reported 2005-10-17 23:23:49 PDT
String slice() testcases are not passing. attached KJS patch fixes it. It should be fairly easy to merge by hand.
Attachments
patch to fix slice() in KJS (1.20 KB, patch)
2005-10-17 23:24 PDT, George Staikos 		no flags
DetailsFormatted DiffDiff
Revised Patch (1.92 KB, patch)
2005-10-24 20:13 PDT, Geoffrey Garen 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
George Staikos
Comment 1 2005-10-17 23:24:17 PDT
Created attachment 4394 [details] patch to fix slice() in KJS
Maciej Stachowiak
Comment 2 2005-10-17 23:31:53 PDT
Comment on attachment 4394 [details] patch to fix slice() in KJS George and I discussed how it would be safer against extreme values ot use toInteger and leave things as double until after the range check, but I think it's still an improvement to merge this fix from KJS.
Darin Adler
Comment 3 2005-10-18 09:55:30 PDT
Comment on attachment 4394 [details] patch to fix slice() in KJS The standard is clear on this point: toUInt32 is incorrect, toInteger is correct. But that's a bug that can be fixed separately.
Geoffrey Garen
Comment 4 2005-10-24 19:42:41 PDT
Filed a new bug to track using toInteger: 5490.
Geoffrey Garen
Comment 5 2005-10-24 20:13:14 PDT
Created attachment 4466 [details] Revised Patch Psych. Decided to fix it myself here. Also fixed the end == undefined case.
Darin Adler
Comment 6 2005-10-25 08:16:52 PDT
Comment on attachment 4466 [details] Revised Patch There are a few examples here of braces for single lines. That doesn't match our coding style. Also, I prefer just doing a return to setting the value of String. Finally, I'd like test cases for what you fixed here. But it looks good to me, so review+ anyway.
Geoffrey Garen
Comment 7 2005-11-05 21:55:32 PST
I removed the single braces and added a layout test for the cases not covered by the mozilla test suite before landing. I felt iffy about returning immediately rather than setting result, though. Setting result is the idiom for StringProtoFuncImp::callAsFunction. If we don't like that idiom, I think we should file a separate bug and change all cases of it in the function, rather than creating an idiosyncracy for this one case.
Note You need to log in before you can comment on or make changes to this bug.