[webkit-reviews] review denied: [Bug 6282] Adding new Option with new Option(text, value, defaultSelected, selected) fails to update selectedIndex : [Attachment 8143] Proposed fix

[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 8143: Proposed fix
http://bugzilla.opendarwin.org/attachment.cgi?id=8143&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

+	  m_element->setSelectedIndex( u );

We don't use parentheses around arguments like this. It should just be "(u)".

This patch needs a test; we don't land bug fixes without tests. We need a test
that demonstrates that this works, and also that demonstrates that modifying
existing elements does *not* change the selected index (to test the "diff >= 0"
in this patch).

Finally, in the future if not in this patch, I'd like to see a lot more of this
in HTMLSelectElement. We don't want functions like this in the binding itself.
JSHTMLSelectCollection::put should deal only with the conventions of how this
feature works JavaScript-wise. Code like adding dummy option elements to padd
out the array and selecting the newly-created element should be in
HTMLSelectElement.

This function should be something like:

    if (propertyName == "selectedIndex")
	m_element->setSelectedIndex(value->toInt32(exec));
    else if (propertyName == lengthPropertyName)
	m_element->setLength(value->toInt32(exec));
    else {
	bool ok;
	unsigned i = propertyName.toUInt32(&ok);
	if (ok)
	    m_element->setOption(i, toNode(value));
    }

It might have to be slightly more complicated than that, but should not be a
lot more. The logic that's really about how select elements behave should be in
HTMLSelectElement rather than the JavaScript binding.

review- because of the lack of a test.