[webkit-reviews] review denied: [Bug 6282] Adding new Option with new Option(text, value, defaultSelected, selected) fails to update selectedIndex : [Attachment 8172] New patch after feedback from Darin

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6282: Adding new Option with new Option(text, value, defaultSelected,
selected) fails to update selectedIndex
http://bugzilla.opendarwin.org/show_bug.cgi?id=6282

Attachment 8172: New patch after feedback from Darin
http://bugzilla.opendarwin.org/attachment.cgi?id=8172&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The structure of the new code looks good. Formatting needs a bit of
improvement. Also, the code that's being moved into HTMLSelectElement has some
old sloppiness that could be fixed while moving it.

We use "unsigned" rather than "unsigned int". We use "ExceptionCode& exception"
rather than "ExceptionCode &exception". We use "HTMLElement*" rather than
"HTMLElement *".

+  if ( propertyName == "selectedIndex" )
     m_element->setSelectedIndex( value->toInt32( exec ) );
+  else {
     int exception = 0;
+    // resize ?
+    if (propertyName == lengthPropertyName) {

I'd like to see the above use else if and have a separate section for each
property. I don't think it does use good to share a single exception variable.

+      if( value->getUInt32(newLen)) {

Parenthesis placed wrong here.

+      unsigned int i = propertyName.toUInt32(&ok);

Just unsigned will do.

I'm not sure why we're using getUInt32 for setting length, toUInt32 for setting
by index, and toInt32 for selectedIndex. We should test with non-integer values
that can be converted to integers to see how this should work. The difference
between getUInt32 and toUInt32 is that the former will not do type conversions.


+	   m_element->setOption(i, toNode(value), exception);

We should test to see that when the value is something other than an option
that we want to do nothing. Instead we should consider doing a remove.

I also think we may want to change HTMLSelectElement::setOption to take a
HTMLOptionElement* parameter instead of a Node* and put the type checking in
the language binding. That's parallel with what we usually do for DOM elements.


+    int diff = int(u) - length();

Should just be u - length() -- no need to cast to int. But if u is a very large
value this will underflow. In general it would be best to put some kind of
limit on this so we won't loop forever putting a huge number of option elements
in if the passed-in value is a huge number.

+    int diff = length() - newLen;

Same issues. We could get an underflow if newLen is a huge number.

+    else // remove elements
+	 while (diff-- > 0)
+	     remove(newLen);

This code would be faster if it always removed from the end.

Could save a little code in HTMLSelectElement::setOption by calling setLength(u
- 1) in the case where diff > 0 instead of having our own "add dummy options"
loop.

In declarations we normally leave out parameter names exception when they make
things clearer.

    void setOption(unsigned index, HTMLOptionElement*, ExceptionCode&);
    void setLength(unsigned, ExceptionCode&);

In the case of setLength, we know that the single parameter is the length.

As far as test cases are concerned, a good test needs to test all the code
paths:

    1) setting length to a larger length
    2) setting length to a smaller length
    3) setting length to the same length
    4) setting length to non-integer values (null, undefined, non-numeric
string, floating point number)
    5) setting an element by index past the end of the current list
    6) setting an existing element by index
    7) trying to set an element that's not an option element (null, undefined,
other kinds of objects, other kinds of elements)

The test should verify how the selected index is affected by these various
mutations, and also how the options in the select are affected. Need to test
both a single-selection select and a multiple-selection select.

A truly thorough test would also monitor DOM mutation events to see things like
whether a DOM element is removed and then added in cases like (6) above, but I
don't think that's terribly important. And test behavior when a DOM mutation
event handler completely changes around the list. Can you make one that causes
setLength to infinite loop?

review- because there's no test yet.