If there is more exception, it will not stop at the first, but the return value will be the first exception.
Created attachment 226105 [details] Patch v1 If I add an if statement before the first if, we can check that there were any exception. If it was, drop a break, and the result will be this exception.
It may be worth investigating implementing min/max in JS
(In reply to comment #2) > It may be worth investigating implementing min/max in JS I had tested that the original method will not stop at the first exception, but the return value will be the first. So the patch will fix this problem.
Comment on attachment 226105 [details] Patch v1 This fix looks just right. But we need to add a test case too, so someone doesn’t break it in the future.
(In reply to comment #4) > (From update of attachment 226105 [details]) > This fix looks just right. But we need to add a test case too, so someone doesn’t break it in the future. Hi! I'm sorry, but there is no way to show sideEffect, because the toNumber will return 0.0, after an exception, so complex objects will return 0.0 after an exception... references are there: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp?rev=164904&order=name#L1562 http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp?rev=164904&order=name#L1350
(In reply to comment #5) > I'm sorry, but there is no way to show sideEffect, because the toNumber will return 0.0, after an exception, so complex objects will return 0.0 after an exception... That’s wrong. The issue is not about a return value. It’s about whether the valueOf code runs at all.
Created attachment 226977 [details] Patch
Comment on attachment 226977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226977&action=review > Source/JavaScriptCore/ChangeLog:9 > + This required fleching out a bit more of our builtins logic *fleching*? > Source/JavaScriptCore/builtins/Math.js:30 > + value = +value; Won't this mean that min(-0, +0) will return +0? I think the spec says it should return -0. > Source/JavaScriptCore/builtins/Math.js:47 > + value = +value; Won't this mean that min(-0, -1) will return +0? I think the spec says it should return -0.
Comment on attachment 226977 [details] Patch Attachment 226977 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5475724035096576 New failing tests: js/math.html
Created attachment 226998 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 226977 [details] Patch Attachment 226977 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4663751877853184 New failing tests: js/math.html
Created attachment 227000 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 226977 [details] Patch Attachment 226977 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6182673236099072 New failing tests: js/math.html
Created attachment 227002 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 226977 [details] Patch Attachment 226977 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4975311221424128 New failing tests: js/math.html
Created attachment 227004 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Oliver: Is this something you plan on completing? Or should we close the bug as NTBF?
Comment on attachment 226977 [details] Patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.