Summary: | Inconsistencies in JS array extras with Mozilla implementation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Walden (remove +bwo to email) <jwalden+bwo> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, gavin.sharp | ||||||||
Priority: | P2 | ||||||||||
Version: | 523.x (Safari 3) | ||||||||||
Hardware: | All | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
Jeff Walden (remove +bwo to email)
2007-03-04 02:11:42 PST
Created attachment 13470 [details]
Patch
Picking the reviewer for the original extras implementation, let me know if someone else should do this...
I also have ECMAScript-262 ed. 3 style specs, since much of the code in some of the examples replicates specification-defined operators like ToNumber. I currently have these specs at <http://web.mit.edu/jwalden/www/array-extras.js>. (In reply to comment #2) > I also have ECMAScript-262 ed. 3 style specs, since much of the code in some of > the examples replicates specification-defined operators like ToNumber. I > currently have these specs at <http://web.mit.edu/jwalden/www/array-extras.js>. Thanks for the patch, Jeff! There are a few things needed before this may be landed: - Create or update existing layout tests that fail before the patch is applied, and pass after the patch is applied. I know you don't have a Mac, but these particular layout tests should run in Mozilla/Firefox (not all layout tests will work this way), and you should be able to add tests for the changed behavior that may be verified later by someone with a Mac. If there are any Mac computer labs at MIT, you should be able to write/update these tests quite easily: LayoutTests/fast/js/array-every.html LayoutTests/fast/js/array-float-delete.html LayoutTests/fast/js/array-foreach.html LayoutTests/fast/js/array-index-immediate-types.html LayoutTests/fast/js/array-indexof.html LayoutTests/fast/js/array-join-bug-11524.html LayoutTests/fast/js/array-lastIndexOf.html LayoutTests/fast/js/array-some.html LayoutTests/fast/js/array-tostring-ignore-separator.html - A changelog entry (generated by prepare-ChangeLog in the WebKitTools/Scripts directory, then edited to include bug information and a description of the fix). - A patch created by the WebKitTools/Scripts/svn-create-patch script (after the above changes) that includes the ChangeLog entries for JavaScriptCore and LayoutTests, the layout test changes, and the code changes. Thanks again! Comment on attachment 13470 [details]
Patch
Thanks. This help is quite welcome!
It's hard to read this patch because it has so much context! I'm really used to the plain old "-u".
The code change looks fine, but we need some test cases and a change log before we can review+ this.
Created attachment 13476 [details] Full patch (but haven't attempted to run the tests yet) (In reply to comment #4) > It's hard to read this patch because it has so much context! I'm really used > to the plain old "-u". Not overdoing context is good, but since -p was useless I wanted enough to show the names of the methods associated with each change. > The code change looks fine, but we need some test cases and a change log > before we can review+ this. I whipped up some tests/changes earlier tonight as a break from classwork. I'm pretty sure the changes are correct, except possibly with respect to amounts of (trailing) whitespace (ugh). I should have access to a Mac tomorrow night, so hopefully I'll be able to build/verify the tests then if no one else has (hence no review flag yet). By the way, these methods are all tested in Mozilla's JS test suite, which is being reorganized to separate Moz-specific behaviors from ECMA-262 canon to aid use by other implementations. (There are nascent plans to try to make some sort of generalized ECMA-262 test suite, but that's a while away for now.) Except for forEach returning undefined, I'd bet we have everything else tested -- resyncing wouldn't be a bad idea if you have the time, as a further correctness check. I wrongly expected the |getProperty()| function in JavaScriptCore returned NULL only for non-existent properties and not for properties with value set to |undefined|, so as written the patch doesn't pass the tests in it (even ignoring the whitespace and formatting flubs in it). I need to use a getProperty/hasProperty pair to fix the problem. Hopefully I'll have time to fix that by the end of the week, but I doubt it'll be any sooner. Created attachment 13786 [details]
Complete patch
Returning false for an array index whose element is undefined seems to be a bug, because hasProperty just returns the result of getPropertySlot.
This patch includes tests, passes all of them, and regresses no existing tests.
Comment on attachment 13786 [details]
Complete patch
r=me
|