[webkit-reviews] review denied: [Bug 5227] Array indexOf() extension for JavaScript 1.5 Core : [Attachment 4157] Implements indexOf, addressing all concerns hopefully :-D

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Justin Haygood
<justin at xiondigital.net>'s request for review:
Bug 5227: Array indexOf() extension for JavaScript 1.5 Core
http://bugzilla.opendarwin.org/show_bug.cgi?id=5227

Attachment 4157: Implements indexOf, addressing all concerns hopefully :-D
http://bugzilla.opendarwin.org/attachment.cgi?id=4157&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for your work on this! There are still a few things to fix and
investigate.

JavaScript method implementations should almost never look at the size of the
args array. That's because most methods ignore extra arguments. By checking
specifically for a size of 2, this method won't ignore such extra arguments
properly.

We need to test Gecko with extra arguments to see what the desired behavior is
for compatibility.

I'd suggest simply checking args[1] to see if it's undefined, rather than
looking at args.size(). The list class always gives undefined rather than doing
something bad when you use an index greater than the end of the list, for just
this reason.

The code to call throwError should return after the call. Despite its name,
throwError does not do a C++ throw, and we don't want to fall into the
subsequent code.

There's no need for a break after a return statement, so that should be
removed.

We need a test case for when indexOf is called with no arguments at all. Does
it find the index of "undefined" in the test array in that case or does it do
something else? What about when the looked-for thing is "null"? Does that match
"undefined" or not? We also need test cases for when it's called with extra
arguments.

I also think it's slightly ugly to have a local variable called fromIndex yet
use it for indexing through the entire array in a loop. Instead perhaps it
should be named something like "i" or "index".

We need a test case for when the index is a very large number, too large to fit
in a 32-bit integer, and a very small number, too small to fit. Also for
negative and positive infinity as well as NaN and negative 0. Note how
functions like splice use a double rather than an int to hold the index when
doing the range check and how they use toInteger rather than toUInt32. There's
a reason for that, which is handling very large or very small numbers; toUInt32
will instead return 0 or a value that's modulo the range of 32-bit integers,
which is almost certainly incorrect.