[Webkit-unassigned] [Bug 6282] Adding new Option with new Option(text, value, defaultSelected, selected) fails to update selectedIndex

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=6282





------- Comment #7 from rwlbuis at xs4all.nl  2006-05-10 00:37 PDT -------
(In reply to comment #5)
> (From update of attachment 8172 [edit])
> 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.

I agree.

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

I tried to address these points in the latest patch.

> +  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.

Should be fixed now.

> +      if( value->getUInt32(newLen)) {
> 
> Parenthesis placed wrong here.

Fixed. I tried to fix similar issues in the code too.

> +      unsigned int i = propertyName.toUInt32(&ok);
> 
> Just unsigned will do.

Fixed.

> 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.

I did not change this.

> +          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.

Are you sure about the 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.

I tried to fix it like described.

> +    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.

I have put a limit on it, not sure if it is unrealisticly high, but any other
limit
sounds a bit arbitrary to me, unless there is some specific value in the spec.

> +    int diff = length() - newLen;
> 
> Same issues. We could get an underflow if newLen is a huge number.

Also tried to fix it.

> +    else // remove elements
> +        while (diff-- > 0)
> +            remove(newLen);
> 
> This code would be faster if it always removed from the end.

I do not see this?

> 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.

Fixed.

> 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.

Fixed.

> 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.

I will try to come up with a testcase later
Cheers,

Rob. 


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.