[webkit-reviews] review denied: [Bug 3667] Core JavaScript 1.5 Reference:Objects:Array:forEach : [Attachment 2569] Patch that adds forEach Javascript 1.5 functionality to JavascriptCore

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Francisco Tolmasky
<tolmasky at gmail.com>'s request for review:
Bug 3667: Core JavaScript 1.5 Reference:Objects:Array:forEach
http://bugzilla.opendarwin.org/show_bug.cgi?id=3667

Attachment 2569: Patch that adds forEach Javascript 1.5 functionality to
JavascriptCore
http://bugzilla.opendarwin.org/attachment.cgi?id=2569&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Patch looks good.

Need a few things before we can take it, though:

    1) We need a layout test to land along with the bug fix. It should test
both the 1 argument and 2 argument forms, as well as cases where the function
raises an exception. In addition it should test the return value and passing
strange values, including both "null" and "undefined" for both parameters.
    2) The check for thisObject should be changed: The way args works, anything
past the end is "undefined", so the code should do an "undefined or null" check
instead of the size check so that undefined will be handed the same way null
is. I'd suggest args[1].imp()->isUndefinedOrNull().
    3) The for statement uses "unsigned int" where we typically use "unsigned"
and is missing a space between the for and the "(".
    4) The for loop continues to iterate even if the function called raises an
exception. I think it should probably break if there's an exception; need a
test case for that.

Looking good. It will be great to have this implemented.