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

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Mon Jul 3 18:02:14 PDT 2006

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

Attachment 9174: Patch v2

------- 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)
	if (lengthValue > static_cast<double>(UINT_MAX))
	    newLength = UINT_MAX;
	    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.


More information about the webkit-reviews mailing list