Bug 9179 – Implement select.options.add() method

• Component: DOM
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

This appears to be an IE extension of the select.options array.

This is the first (and so far only) error reported to the JavaScript console when opening the Kupu nightly URL (see above).

[David Kilzer (:ddkilzer)]

Kupu nightly that causes error:

http://debris.demon.nl:7080/kupu_nightly/kupu/common/kupu.cgi

[David Kilzer (:ddkilzer)]

Created attachment 8941 [details]
Patch v1 (for review only!)

This is a first-pass patch at the options.add() implementation.  I'm only looking for feedback now, and I expect this patch to get an r-.  This patch should NOT be applied to SVN.

Changes:

- Removed poorly-named HTMLSelectCollection class from kjs_html.cpp.  Added IDL for HTMLOptionsCollection.
- Fixed names of "HTMLCollectionProto*" items in kjs_html.cpp to add "JS" prefix to match naming conventions.
- Added new "HasCustomIndexSetter" to CodeGenereratorJS.pm which expects an indexSetter() method to be available on the JS object.  (I don't really like the name "HasCustomIndexSetter" because there is no "HasIndexSetter" default implementation, but I wasn't sure if that would happen when HTMLCollection was extracted from kjs_html.cpp.)
- Added new lookupPutWithIndex() template in lookup.h (blatantly copied from lookupPut() template).  I don't like the code duplication, but wasn't sure of a better way to do this off the top of my head.
- All current layout tests pass with this patch; only the new options.add() feature is not fully tested (see below).

Known issues:

- Layout tests are not complete and are still failing in some cases.  I will flesh them out per the MS spec and do more testing on Firefox and MSIE to make sure I match their behavior.
- There seems to be some code duplication between the HTMLSelect and HTMLCollection regarding the management of the Option element list.  I'm wondering if I should clean that up while I'm digging around in this part of the code?

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> - There seems to be some code duplication between the HTMLSelect and
> HTMLCollection regarding the management of the Option element list.  I'm
> wondering if I should clean that up while I'm digging around in this part of
> the code?

This is why there is only a setLength() method on HTMLOptionsCollection and not a length(), since the HTMLCollection::length() method handles that.  I could have added HTMLOptionsCollection::length() and called the method on the HTMLSelectElement instead, but wasn't sure which way was better (due to the code duplication).

[David Kilzer (:ddkilzer)]

Copying Anders since he's been doing and reviewing a lot of the IDL work lately.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Known issues:
> 
> - Layout tests are not complete and are still failing in some cases.  I will
> flesh them out per the MS spec and do more testing on Firefox and MSIE to make
> sure I match their behavior.

That is, only the NEW layout tests for the options.add() feature are failing.  The existing layout tests all pass.

[Darin Adler]

Comment on attachment 8941 [details]
Patch v1 (for review only!)

Nice!

There's code here that says WebCore::Node inside namespace WebCore -- no need for that. Also formatted wrong (* should be next to node).

Why is kjs_binding.h now including Node.h? I'd like to avoid that if at all possible! Can the .cpp file that depends on Node.h include it instead, or does the header now somehow depend on it?

+    // FIXME: Does this work without the if/else statement?

What does that comment mean?

+        select->setOption(unsigned(index), static_cast<HTMLOptionElement*>(node.get()), ec);

I'm not fond of the cast syntax here. Also, what does this function do if the node is not an option element? Casting an object that's not of the right type can end up creating memory-trashing bugs, so there needs to be some level of code that checks (or somehow guarantees) the type before casting.

+    int selectedIndex();

Should be const.

+    void setSelectedIndex(int index);

Should omit the word "index" here.

I think it's a little ugly to have to add a new variant of lookupPut here. Especially as it hard-wires the name indexSetter, which doesn't exist anywhere in KJS. One possibility would be to make a version of lookupPut that just returns a boolean, false, if it's not found in the lookup table. Then, the call to indexSetter could be in the WebCore (generated) code. I think that would be a little cleaner, although perhaps a tiny bit slower since there'd be an additional if statement.

+    HTMLOptionsCollection* impl = static_cast<HTMLOptionsCollection*>(JSHTMLOptionsCollection::impl());

How about using a different name for the local variable than the function so we can avoid that big JSHTMLOptionsCollection::impl() mouthful? Maybe "imp"? And maybe the generated code could get the type right so we wouldn't need casting and local variables?

+        newLength = unsigned(floor(lengthValue));

I know this was not new code, but the call to floor here is unnecessary, since floor is what a cast to unsigned will do when the number is greater than or equal to zero. It also would be good to have a check for a value too large to fit in an unsigned and do something predictable in that case rather than just truncating the number.

+    if (value->isUndefinedOrNull()) {
+        base->remove(index);
+    } else {

We don't use braces around one-line if statements.

Why does getHTMLOptionsCollection still take an HTMLCollection parameter instead of an HTMLOptionsCollection parameter? What if the caller passes an HTMLCollection that's not an HTMLOptionsCollection? Again, casting in such cases is dangerous.

[David Kilzer (:ddkilzer)]

While writing test cases for this bug, I found a bug that crashes Firefox:

https://bugzilla.mozilla.org/show_bug.cgi?id=342677

[David Kilzer (:ddkilzer)]

Anders, Eric, I need help/an idea on how to solve a specific problem.

The options.add() function can take one or two arguments, the second one being optional.  The second argument is an integer, but when the second argument (through JavaScript) is not provided, it is sent as zero, but that's indistinguishable from a real argument.

What's the best way to handle a situation like this in the IDL?  Is there a way to send an Integer object as a JSValue to the Node class method (HTMLOptionsCollection::add()) instead of an 'int' so I know when it's not set?  Or is there a way to pass in the number of original arguments sent via the JavaScript function?  Thanks!

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> What's the best way to handle a situation like this in the IDL?  Is there a way
> to send an Integer object as a JSValue to the Node class method
> (HTMLOptionsCollection::add()) instead of an 'int' so I know when it's not
> set?  Or is there a way to pass in the number of original arguments sent via
> the JavaScript function?  Thanks!

A commit log comment for Bug 8227 mentions that method overloading may be a better long-term approach to optional arguments in JavaScript functions, which is the same conclusion I reached after thinking about this problem for a few days.  I will try to fix the above problem by teaching CodeGeneratorJS.pm about overloaded methods.  Here's Maciej's comment from that commit (r13726):

* bindings/scripts/CodeGeneratorJS.pm: Added support for ConvertUndefinedToTrue, for benefit of scroll methods where omitted arguments should be treated as true.  However, maybe explicit overloading in the IDL would be a better long-term approach for optional arguments.

[David Kilzer (:ddkilzer)]

Committed revision 15137.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> Committed revision 15137.

Whoops!  Wrong bug!  That was meant for Bug 9654.
 

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 8941 [details] [edit])

Changes in the next patch:

> There's code here that says WebCore::Node inside namespace WebCore -- no need
> for that. Also formatted wrong (* should be next to node).

Fixed in next patch by using generated toHTMLOptionElement() converter method.

> Why is kjs_binding.h now including Node.h? I'd like to avoid that if at all
> possible! Can the .cpp file that depends on Node.h include it instead, or does
> the header now somehow depend on it?

This was fixed when I added the "GenerateNativeConverter" attribute to HTMLOptionElement.idl and updatedTypeCanFailConversion() in CodeGeneratorJS.pm to include HTMLOptionElement.

> +    // FIXME: Does this work without the if/else statement?
> 
> What does that comment mean?

This was an implementation FIXME comment since I didn't have the full test suite completed.  It has been removed.

> +        select->setOption(unsigned(index),
> static_cast<HTMLOptionElement*>(node.get()), ec);
>
> I'm not fond of the cast syntax here. Also, what does this function do if the
> node is not an option element? Casting an object that's not of the right type
> can end up creating memory-trashing bugs, so there needs to be some level of
> code that checks (or somehow guarantees) the type before casting.

This was also fixed with the native converter for HTMLOptionElement, which does a safe type check in the generated code before passing the argument into this method.  It sets the value of the element to zero if it isn't an HTMLOptionElement.

> +    int selectedIndex();
>
> Should be const.

Fixed.

> +    void setSelectedIndex(int index);
> 
> Should omit the word "index" here.

Fixed.

> I think it's a little ugly to have to add a new variant of lookupPut here.
> Especially as it hard-wires the name indexSetter, which doesn't exist anywhere
> in KJS. One possibility would be to make a version of lookupPut that just
> returns a boolean, false, if it's not found in the lookup table. Then, the call
> to indexSetter could be in the WebCore (generated) code. I think that would be
> a little cleaner, although perhaps a tiny bit slower since there'd be an
> additional if statement.

Fixed by extracting a lookupPut() method that returns a bool if the entry in the lookup table is not found.

> +    HTMLOptionsCollection* impl =
> static_cast<HTMLOptionsCollection*>(JSHTMLOptionsCollection::impl());
> 
> How about using a different name for the local variable than the function so we
> can avoid that big JSHTMLOptionsCollection::impl() mouthful? Maybe "imp"? And
> maybe the generated code could get the type right so we wouldn't need casting
> and local variables?

Fixed in JSHTMLOptionsCollectionCustom.cpp as well as CodeGeneratorJS.pm for all generated code.

> +        newLength = unsigned(floor(lengthValue));
> 
> I know this was not new code, but the call to floor here is unnecessary, since
> floor is what a cast to unsigned will do when the number is greater than or
> equal to zero. It also would be good to have a check for a value too large to
> fit in an unsigned and do something predictable in that case rather than just
> truncating the number.

Removed floor() method.  Added check for UINT_MAX overflow.

> +    if (value->isUndefinedOrNull()) {
> +        base->remove(index);
> +    } else {
> 
> We don't use braces around one-line if statements.

Fixed this as well as some other '*' operator placement issues.

> Why does getHTMLOptionsCollection still take an HTMLCollection parameter
> instead of an HTMLOptionsCollection parameter? What if the caller passes an
> HTMLCollection that's not an HTMLOptionsCollection? Again, casting in such
> cases is dangerous.

Fixed the collection argument to the getHTMLOptionsCollection() method.

[David Kilzer (:ddkilzer)]

Created attachment 9169 [details]
Test results from Firefox 1.5.0.4

This page contains the test results for Firefox 1.5.0.4.  They all pass with the following exceptions:

- Firefox 1.5.0.4 crashes if 'null' is passed into the options.add() method.  This will be fixed in Firefox 1.5.0.5:

https://bugzilla.mozilla.org/show_bug.cgi?id=342677

- Firefox 1.5.0.4 does not throw a "type mismatch" exception for passing primitive types into the options.add() method.  This will also be fixed by the patch for 342677.

- Firefox 1.5.0.4 doesn't support the innerText property, so the test was changed to textContent to make the test pass.  (MSIE 6 doesn't support textContext.)  Filed a Firefox bug to cover this.

https://bugzilla.mozilla.org/show_bug.cgi?id=343357

[David Kilzer (:ddkilzer)]

Created attachment 9170 [details]
Test results from MSIE 6 (via MS Word)

This page contains the test results for MSIE 6.  The tests all pass with the following exceptions:

- MSIE 6 doesn't allow you to call options.add() after setting 'value' or 'innerText' properties on the element.  (Firefox 1.5.0.4 does, though.)

Note that I had to copy and paste the results into MS Word (then save the Word document as HTML) to get the results in HTML.

[David Kilzer (:ddkilzer)]

Created attachment 9174 [details]
Patch v2

Updates since Patch v1 not covered in Comment #12:

- Implemented IDL function overloading by introducing the "Optional" extended attribute for parameters.  Updated the GenerateImplementation() subroutine in CodeGeneratorJS.pm to incrementally check the argument list count as it processes optional parameters to determine when the argument list is complete.  Extracted the GenerateImplementationFunctionCall() subroutine.  Note that the "Optional" attribute only has to be listed on one argument since all others following it are assumed to be optional as well.

- Added ChangeLog entries.

[David Kilzer (:ddkilzer)]

See Bug 9717 for an example use of the new "Optional" parameter attribute.

[Darin Adler]

Comment on attachment 9174 [details]
Patch v2

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

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> (From update of attachment 9174 [details] [edit])
> 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);
>     }

This change causes a regression in fast/dom/select-selectedIndex-multiple.html.  I will investigate why.

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

I'll write some more tests before this patch lands.

[David Kilzer (:ddkilzer)]

Comment on attachment 9174 [details]
Patch v2

I've got some improved tests for this patch which changed JSHTMLOptionsCollectionCustom.cpp.

[David Kilzer (:ddkilzer)]

Created attachment 9325 [details]
Patch v3

Changes since Patch v2:

- Added more test cases specifically targeting -Infinity, NaN and +Infinity in fast/dom/select-selectedIndex-multiple.html and fast/js/select-options-add.html.
- Made sure fast/dom/select-selectedIndex.html was kept in sync with test cases from select-selectedIndex-multiple.html.
- Overloaded KJS::JSValue::toInt32() to take a boolean 'Ok' argument to flag when zero is returned for a float with a value of +-Infinity or NaN.
- Setting the options.length to a negative value now throws an exception.  (This mimics MSIE 6, but differs from Firefox 1.5.0.x.)
- Passing anything other than an option element to options.add() causes an exception to be thrown for all cases now (including +-Infinity and NaN, which were being valued to zero before; this is where the new toInt32() method is used to trap these values as errors).
- Changed overloaded function feature in CodeGeneratorJS.pm to use "argCount <" instead of "argCount ==" comparison.
- Changed CanFailTypeConversion() in CodeGeneratorJS.pm to return true for "unsigned long" and "long" types.  (They now use the new toInt32() overloaded method.)
- Changed JSValueToNative() in CodeGeneratorJS.pm to use new toInt32() overloaded method.

All layout tests still pass with these changes.  Will upload Firefox 1.5.0.5rc3 and MSIE 6 test results next.

[David Kilzer (:ddkilzer)]

Created attachment 9326 [details]
Firefox 1.5.0.5rc3 results for fast/js/select-options-add.html

Differences from WebKit with Patch v3:

- Calling options.add() with an index value of -Infinity, NaN or +Infinity causes the script to halt execution rather than throwing an exception (2.9, 2.10, 2.11).  See this bug for details:  https://bugzilla.mozilla.org/show_bug.cgi?id=344067.  Note that this is fixed in the latest 3.0a1 Firefox nightly as of July 9, 2006.

[David Kilzer (:ddkilzer)]

Created attachment 9327 [details]
Firefox 1.5.0.5rc3 results for fast/js/select-selectedIndex-multiple.html

Differences from WebKit with Patch v3:

- Setting options.length to -1 does not throw an exception in Firefox 1.5.0.5rc3.  In WebKit, it will throw an exception.  (MSIE 6 also throws an exception.)  See this bug for more details:  https://bugzilla.mozilla.org/show_bug.cgi?id=344060.  (At least one person doesn't think it should throw an exception on the Moz side.)

[David Kilzer (:ddkilzer)]

Created attachment 9328 [details]
Firefox 1.5.0.5rc3 results for fast/js/select-selectedIndex.html

Differences from WebKit with Patch v3:

- Setting options.length to -1 does not throw an exception in Firefox 1.5.0.5rc3.  In WebKit, it will throw an exception.  (MSIE 6 also throws an exception.)  See this bug for more details:  https://bugzilla.mozilla.org/show_bug.cgi?id=344060.  (At least one person doesn't think it should throw an exception on the Moz side.)

[David Kilzer (:ddkilzer)]

Created attachment 9329 [details]
MSIE 6 results for fast/js/select-options-add.html

Differences from WebKit with Patch v3:

- None.

[David Kilzer (:ddkilzer)]

Created attachment 9330 [details]
MSIE 6 results for fast/js/select-selectedIndex-multiple.html

Differences from WebKit with Patch v3:

- Setting 'select.options[N] = undefined' causes the script to halt execution at that point.  WebKit matches Firefox in this case, though, and does nothing.

[David Kilzer (:ddkilzer)]

Created attachment 9331 [details]
MSIE 6 results for fast/js/select-selectedIndex.html

Differences from WebKit with Patch v3:

- Setting 'select.options[N] = undefined' causes the script to halt execution at that point.  WebKit matches Firefox in this case, though, and does nothing.
- The select.selectedIndex value differs in many tests, but WebKit matches Firefox behavior in every case.

[Darin Adler]

Comment on attachment 9325 [details]
Patch v3

This seems nearly perfect even though I have a few minor quibbles.

I'm concerned about adding another level of function call for every caller of toInt32, which might be hot. Maybe just repeat the code twice?

I think the bool& parameter's purpose is not self-explanatory and so I think it should have a name in the header.

I don't see the code to set $maybeOkParam!

+    return 1; # 0; # or can it?

The above line of code is confusing, even more than it was before.

This is getting really close, but review- for now for at least some of these questions.

This change looks really great. Lets get it landed!

[David Kilzer (:ddkilzer)]

Created attachment 9339 [details]
Patch v4

Changes since Patch v3:

(In reply to comment #27)
> (From update of attachment 9325 [details] [edit])
> I'm concerned about adding another level of function call for every caller of
> toInt32, which might be hot. Maybe just repeat the code twice?

I hate duplicating code.  I made the one-argument method inline in value.h.  Is that okay?

> I think the bool& parameter's purpose is not self-explanatory and so I think it
> should have a name in the header.

Added.

> I don't see the code to set $maybeOkParam!

This value is set from the return value of TypeCanFailConversion(), which is the line of code in question below.

> +    return 1; # 0; # or can it?
> 
> The above line of code is confusing, even more than it was before.

Sorry...I left the old code in when first attempting to change the value.  I removed "# 0; # or can it?" in this patch.

[Darin Adler]

Comment on attachment 9339 [details]
Patch v4

r=me

[David Kilzer (:ddkilzer)]

Committed revision 15321.

[Timothy Hatcher]

<rdar://problem/4383595>

[David Kilzer (:ddkilzer)]

FWIW, the bug for implementing select.options.remove() is Bug 9683.