Bug 3667 – Core JavaScript 1.5 Reference:Objects:Array:forEach

• Component: JavaScriptCore
• Status: VERIFIED FIXED

[Francisco Tolmasky]

Executes a provided function once per array element.

[Francisco Tolmasky]

Created attachment 2569 [details]
Patch that adds forEach Javascript 1.5 functionality to JavascriptCore

This attachment adds the functionality outlined by Ecma for forEach in the
Array object as found here:
http://developer-test.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Objects:Array:forEach

[Darin Adler]

Comment on attachment 2569 [details]
Patch that adds forEach Javascript 1.5 functionality to JavascriptCore

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.

[Francisco Tolmasky]

Created attachment 2598 [details]
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore

This revised patch fixes the following concerns:
1. Coding Style Guidelines
2. Proper handling of exceptions.
3. Proper handling of optional arguments.

Also changed "thisObject" to "applyThis" so as to not be confused with
"thisObj".

[Francisco Tolmasky]

Created attachment 2600 [details]
Layout tests to test forEach functionality.

This archive contains the layout test for forEach as well as the expected
results and should be places in layout-tests/fast/js/

[Darin Adler]

Comment on attachment 2600 [details]
Layout tests to test forEach functionality.

Very nice looking test case.

[Darin Adler]

Comment on attachment 2598 [details]
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore

A few tiny code formatting issues.

There should only be one space after the colon (":  args[1]").

There should not be a space after the parenthesis in the for.

The if statement is formatting incorrectly:

    if (exec->hadException())

the above is correct style. Patch otherwise looks great!

[Francisco Tolmasky]

Created attachment 2602 [details]
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore

Fixed hasException coding style convention.

[Francisco Tolmasky]

Created attachment 2611 [details]
Patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore

I went ahead and added some and every support.	I also fixed a bug that caused
Safari to crash when you sent in a non-function as the callback, for instance:
myArray.forEach(5).  To check for this I added another test to the layout-tests
(look at other attachments for this).

[Francisco Tolmasky]

Created attachment 2612 [details]
Layout tests to test forEach(), some(), and every() functionality.

I added another test to array-foreach.html to account for a crashing bug. I
also went ahead and wrote comprehensive tests for every and some.  Pretty sure
everything is rigorously tested.

[Darin Adler]

Comment on attachment 2612 [details]
Layout tests to test forEach(), some(), and every() functionality.

Nice test cases.

[Darin Adler]

Comment on attachment 2611 [details]
Patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore

Looks great. There are some minor issues remaining that should be fixed before
we land this.

The URL in the comment only documents forEach, so it's not quite right any
more.

Formatting issues:

    1) Comment inside the case is not indented to match the code.
    2) "if(id" should be "if (id"
    3) single-line if and else, should be broken into multiple lines
    4) "for ( unsigned" should be "for (unsigned"
    5) indenting of if statements at end of loop looks like it's either wrong
or using tabs
    6) no need for else after an if statement that does a break

I think the if statement that sets up the result before the loop would be
clearer if it had a separate case for Some and for Every; the current version
is unnecessarily clever in a way that makes it slightly less clear.

The predicateResult boolean should be declared where it's initialized. There's
no reason to declare it outside the loop.

[Francisco Tolmasky]

Created attachment 2630 [details]
Revised patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore

[Darin Adler]

Comment on attachment 2630 [details]
Revised patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore

There's still a lot of indentation using tabs in the new code in this patch;
need to use spaces instead.

But I'm not going to be a hardass and say review- just because of that. r=me

[Geoffrey Garen]

Created attachment 2708 [details]
patch revised for indentation

Fixing indentation before committing.