[Webkit-unassigned] [Bug 9179] Implement select.options.add() method

[bugzilla-daemon at opendarwin.org]

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





------- Comment #12 from ddkilzer at kilzer.net  2006-07-03 10:18 PDT -------
(In reply to comment #6)
> (From update of attachment 8941 [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.


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