Bug 3667 - Core JavaScript 1.5 Reference:Objects:Array:forEach
Summary: Core JavaScript 1.5 Reference:Objects:Array:forEach
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-06-23 00:17 PDT by Francisco Tolmasky
Modified: 2005-07-03 08:15 PDT (History)
0 users

See Also:


Attachments
Patch that adds forEach Javascript 1.5 functionality to JavascriptCore (2.87 KB, patch)
2005-06-23 00:26 PDT, Francisco Tolmasky
darin: review-
Details | Formatted Diff | Diff
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore (2.90 KB, patch)
2005-06-23 10:51 PDT, Francisco Tolmasky
darin: review-
Details | Formatted Diff | Diff
Layout tests to test forEach functionality. (2.99 KB, application/zip)
2005-06-23 10:55 PDT, Francisco Tolmasky
darin: review+
Details
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore (2.88 KB, patch)
2005-06-23 11:50 PDT, Francisco Tolmasky
no flags Details | Formatted Diff | Diff
Patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore (3.56 KB, patch)
2005-06-23 17:39 PDT, Francisco Tolmasky
darin: review-
Details | Formatted Diff | Diff
Layout tests to test forEach(), some(), and every() functionality. (8.93 KB, application/zip)
2005-06-23 17:43 PDT, Francisco Tolmasky
darin: review+
Details
Revised patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore (3.80 KB, patch)
2005-06-24 11:21 PDT, Francisco Tolmasky
darin: review+
Details | Formatted Diff | Diff
patch revised for indentation (4.05 KB, patch)
2005-06-29 18:25 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Francisco Tolmasky 2005-06-23 00:17:32 PDT
Executes a provided function once per array element.
Comment 1 Francisco Tolmasky 2005-06-23 00:26:20 PDT
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
Comment 2 Darin Adler 2005-06-23 07:06:56 PDT
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.
Comment 3 Francisco Tolmasky 2005-06-23 10:51:54 PDT
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".
Comment 4 Francisco Tolmasky 2005-06-23 10:55:23 PDT
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/
Comment 5 Darin Adler 2005-06-23 11:13:46 PDT
Comment on attachment 2600 [details]
Layout tests to test forEach functionality.

Very nice looking test case.
Comment 6 Darin Adler 2005-06-23 11:14:13 PDT
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!
Comment 7 Francisco Tolmasky 2005-06-23 11:50:41 PDT
Created attachment 2602 [details]
Revised patch that adds forEach Javascript 1.5 functionality to JavascriptCore

Fixed hasException coding style convention.
Comment 8 Francisco Tolmasky 2005-06-23 17:39:28 PDT
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).
Comment 9 Francisco Tolmasky 2005-06-23 17:43:01 PDT
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.
Comment 10 Darin Adler 2005-06-24 08:37:00 PDT
Comment on attachment 2612 [details]
Layout tests to test forEach(), some(), and every() functionality.

Nice test cases.
Comment 11 Darin Adler 2005-06-24 08:45:08 PDT
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.
Comment 12 Francisco Tolmasky 2005-06-24 11:21:11 PDT
Created attachment 2630 [details]
Revised patch that adds forEach(), some(), and every() Javascript 1.5 functionality to JavascriptCore
Comment 13 Darin Adler 2005-06-24 21:27:00 PDT
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
Comment 14 Geoffrey Garen 2005-06-29 18:25:55 PDT
Created attachment 2708 [details]
patch revised for indentation

Fixing indentation before committing.