[webkit-reviews] review granted: [Bug 9179] Implement select.options.add() method : [Attachment 9174] Patch v2

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 9179: Implement select.options.add() method
http://bugzilla.opendarwin.org/show_bug.cgi?id=9179

Attachment 9174: Patch v2
http://bugzilla.opendarwin.org/attachment.cgi?id=9174&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The reason getNumber returns a boolean is so that you don't have to make a
separate call to isNumber. And we don't use the function call syntax for type
casts -- instead we use C++ cast syntax. So this:

+    if (value->isNumber()) {
+	 double lengthValue;
+	 value->getNumber(lengthValue);
+	 if (lengthValue < 0.0)
+	     return;
+	 if (lengthValue > double(UINT_MAX))
+	     newLength = UINT_MAX;
+	 else
+	     newLength = unsigned(lengthValue);
+    }

should be:

    double lengthValue;
    if (value->getNumber(lengthValue)) {
	if (lengthValue < 0.0)
	    return;
	if (lengthValue > static_cast<double>(UINT_MAX))
	    newLength = UINT_MAX;
	else
	    newLength = static_cast<unsigned>(lengthValue);
    }

I'd like to see tests that cover a few more of the branches, for example
setting length to infinity and NaN. The code makes infinite try to set it to
UINT_MAX, NaN sets it to 0, although -1 doesn't change the existing length. Is
that really the behavior we want? If so, then lets make sure tests exercise
those cases and perhaps try them in other browsers.

r=me