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

[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 121272: Patch by tkent 4
https://bugs.webkit.org/attachment.cgi?id=121272&action=review

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


Overall looks good, but there is a implementation mistake (virtual functions in
derived classes), and an unnecessarily complex design.

> Source/WebCore/html/FormAssociatedElement.cpp:45
> +    setForm(0);

At this point, it is too late to call virtual functions in HTMLInputElement;
overrides like HTMLInputElement::willChangeForm and
HTMLInputElement::didChangeForm will not get called. That’s because of how C++
destructors work. We’ll need to do this some other way.

> Source/WebCore/html/FormAssociatedElement.cpp:125
> +    willChangeForm();
> +    m_form = newForm;
> +    didChangeForm();

I think the removeFormElement and registerFormElement code should be moved into
non-virtual functions, or just put inline directly into the setForm function.
Then we can use a single set of virtual functions for both changing forms and
disassociating with forms being destroyed. We should have just willChangeForm
and didChange form.

> Source/WebCore/html/FormAssociatedElement.cpp:157
> +void FormAssociatedElement::willBeUnassociatedFromDestroyingForm()
> +{
> +}
> +
> +void FormAssociatedElement::wasUnassociatedFromDestroyingForm()
> +{
> +    ASSERT(!m_form);
>  }

A couple language nits. The verb for “association is going away” is
“disassociated”. And “destroying form” is a verb phrase, not a noun phrase. So
we probably need to rename these. Also, if we’re asserting !m_form in
wasUnassociatedFromDestroyingForm we could also assert m_form in
willBeUnassociatedFromDestroyingForm.

> Source/WebCore/html/FormAssociatedElement.h:56
> +    void willDestroyForm();

The grammar here gets twisted. Since the element will not be the one destroying
the form, the function name here seems clearly wrong. I think
formWillBeDestroyed is the name we should use.

And then those other functions above can be named:

    willDisassociateFormThatWillBeDestroyed
    didDisassociateFormThatWillBeDestroyed

Long unpleasant names, but the best I could do. But since my other comments say
to remove these functions, I think we’ll be OK.

> Source/WebCore/html/HTMLFormElement.cpp:101
>      for (unsigned i = 0; i < m_associatedElements.size(); ++i)
> -	   m_associatedElements[i]->formDestroyed();
> +	   m_associatedElements[i]->willDestroyForm();

Technically, at this point the form is already partly-destroyed. We can
probably get away with this fuzziness, though.

> Source/WebCore/html/HTMLInputElement.cpp:-123
> -    // Need to remove this from the form while it is still an
HTMLInputElement,
> -    // so can't wait for the base class's destructor to do it.
> -    removeFromForm();

The setForm(0) call will need to be here for the same reason that
removeFromForm needed to be here. The refactoring doesn’t change the basic
truth that when a base class destructor is called, it can’t call virtual
function overrides in derived classes.

I don’t understand why test cases failed to detect this problem.