[webkit-reviews] review denied: [Bug 121586] Fix the HTMLSelectElement.prototype.remove() method : [Attachment 213291] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 121586: Fix the HTMLSelectElement.prototype.remove() method
https://bugs.webkit.org/show_bug.cgi?id=121586

Attachment 213291: Patch
https://bugs.webkit.org/attachment.cgi?id=213291&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213291&action=review


Good thing to fix, but not done quite the right way.

> Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp:93
> -    HTMLOptionsCollection* imp = impl();
> -    JSHTMLSelectElement* base =
jsCast<JSHTMLSelectElement*>(asObject(toJS(exec, globalObject(),
imp->ownerNode())));
> -    return base->remove(exec);
> +    HTMLOptionsCollection& imp = *impl();
> +
> +    // The argument can be an HTMLOptionElement or an index.
> +    if (HTMLOptionElement* option = toHTMLOptionElement(exec->argument(0)))
{
> +	   HTMLSelectElement& select =
*static_cast<HTMLSelectElement*>(imp.ownerNode());
> +	   select.remove(option);
> +    } else
> +	   imp.remove(exec->argument(0).toInt32(exec));
> +    return jsUndefined();

Overloading should be reflected in the real class, not just the binding. The
job of the binding is to get the types converted from JavaScript and get over
to the DOM. So this should be something like this:

    JSValue argument0 = exec->argument(0);
    if (HTMLOptionElement* option = toHTMLOptionElement(argument0))
	impl()->remove(option);
    else
	impl()->remove(argument.toInt32(exec));

The rest of the logic should be in the functions in HTMLOptionsCollection
itself. No accident that JSHTMLSelectElement does it this way.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:44
> +	   static_cast<Element&>(select).remove(ec);

This is the wrong way to do it. We should not cast to Element&. Instead we
should use “using” to make the inherited remove visible in the
HTMLSelectElement class like this:

    using HTMLElement::remove;
    // other remove overloads go here