[webkit-reviews] review denied: [Bug 45719] A radio button not in a Document should not join a radio button group : [Attachment 120535] Patch by tkent

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 45719: A radio button not in a Document should not join a radio button
group
https://bugs.webkit.org/show_bug.cgi?id=45719

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

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


I am not sure we have enough test coverage. Given what I see here in the code,
I suspect that radio buttons end up stuck in groups when they should not be
there.

> Source/WebCore/dom/CheckedRadioButtons.cpp:72
> -void CheckedRadioButtons::removeButton(HTMLFormControlElement* element)
> +void CheckedRadioButtons::removeButton(HTMLInputElement* element)
>  {
> -    if (element->name().isEmpty() || !m_nameToCheckedRadioButtonMap)
> +    if (!shouldMakeRadioGroup(element))
> +	   return;
> +    if (!m_nameToCheckedRadioButtonMap)
>	   return;

I don’t think this function should have been changed, except for changing the
type. There is no harm in removing a button from the group if it’s no longer in
the document. If this is called and the element is no longer in the document,
all the more reason we’d want to remove it from the checked radio button set.
In fact, in HTMLInputElement::removedFromDocument, doesn’t inDocument already
return false?

> Source/WebCore/html/FormAssociatedElement.cpp:125
> +    if (m_form)
> +	   m_form->removeFormElement(this);

I suggest putting this into FormAssociatedElement::willChangeForm.

> Source/WebCore/html/FormAssociatedElement.cpp:128
> +    if (m_form)
> +	   m_form->registerFormElement(this);

I suggest putting this into FormAssociatedElement::didChangeForm.

> Source/WebCore/html/FormAssociatedElement.cpp:136
> +    willChangeForm();

If the form has already been destroyed, then
HTMLInputElement::checkedRadioButtons is going to access a deleted object. So
this code won’t work!

> Source/WebCore/html/HTMLFormControlElement.cpp:53
> +    , FormAssociatedElement()

Should be able to just delete this line entirely.

> Source/WebCore/html/HTMLFormControlElement.cpp:107
>      if (attr->name() == formAttr) {
>	   formAttributeChanged();
> -	   if (!form())
> -	       document()->checkedRadioButtons().addButton(this);
>      } else if (attr->name() == disabledAttr) {

No braces needed any more since this is now a single line if body.

> Source/WebCore/html/HTMLInputElement.cpp:1500
> +    checkedRadioButtons().removeButton(this);

This should call through to the base class.

> Source/WebCore/html/HTMLInputElement.cpp:1505
> +    checkedRadioButtons().addButton(this);

This should call through to the base class.