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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 121792: Patch by tkent 5
https://bugs.webkit.org/attachment.cgi?id=121792&action=review

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


> Source/WebCore/ChangeLog:30
> +	    Do not update m_form except in setForm() and formDestoryed().

Typo: destoryed

> Source/WebCore/ChangeLog:39
> +	   This virtual function is called before the owner form is udpated.

Typo: udpated.

> Source/WebCore/ChangeLog:41
> +	   This virtual function is called after the owner form was udpated.

Typo: udpated.

> Source/WebCore/ChangeLog:43
> +	   - Renamaed from formDestoryed()

Typos: renamaed and destoryed.

> Source/WebCore/dom/CheckedRadioButtons.cpp:70
> +    if (!element->isRadioButton())
> +	   return;

This is not needed for correctness, so I suggest we leave it out.

If you want to keep it in because it’s a performance optimization, consider
that the existing checks are not performance optimizations. The empty name
check below is needed, because the name is the key used in the map, so it's not
a performance optimization. Same for the m_nameToCheckedRadioButtonMap check.

If we do want to improve performance here, then we could do the radio button
check, but we should consider some other things as well:

1) Besides isRadioButton() we could also consider calling checked(), and use
one or both depending on which are more effective at making the code faster.
2) We could change the function so that it only calls name() once, since name
does both a function call and a subsequent virtual function call.
3) HTMLInputElement::name could be optimized by overriding the non-virtual name
function inherited from FormAssociatedElement.

For (2), the name could be put in a local variable:

    const AtomicString& name = element->name();

since it’s used twice in the function.

For (3), HTMLInputElement could cut out the function overhead adding a
non-virtual name function. Any code that has an HTMLInputElement* would then
get the better performance automatically. The simplest way to write it would
be:

    const AtomicString& name() { return HTMLInputElement::formElementName(); }

This would compile in a non-virtual call to the formElementName function, which
is fine because classes derived from HTMLInputElement do not override
formElementName. Or we could optimize even further by creating a function in
HTMLInputElement that just returns m_name, since this code handles null and
empty names identically. The name/formControlName function has logic to turn a
null string into an empty string and we have no need for that here. These
functions (either version) could be inline, and the latter one would simply be
a direct accessor to a data member. Or we could change
FormAssociatedElement::name and FormAssociatedElement::formControlName so that
it's legal to return the null string and change the call sites that don't
already cope with that.

In a mostly unrelated note, when looking at this I noticed a peculiarly-named
public function, setDefaultName, that is called only by the HTMLIsIndexElement
constructor. We should probably make that function protected. And further, it’s
only safe to call that function if the object is not a checkbox, so we might
want to assert that. And it’s only correct to call it on an object that doesn’t
already have a name, so we might want to assert that m_name is null. And
finally, this doesn’t actually work correctly if a name is added and then
subsequently removed from the isindex element. That’s probably unimportant
because of how rare it is to use the isindex element at all, but annoying that
it’s incorrect. That’s the reason the function name is so bad: It doesn’t
actually set a default name, just sets an initial name.

> Source/WebCore/html/FormAssociatedElement.cpp:83
>	   // Resets the form owner at first to make sure the element don't
>	   // associate any form elements when there is no element which has
>	   // the given ID.

This comment no longer makes sense.

> Source/WebCore/html/FormAssociatedElement.cpp:88
>	   Element* formElement =
element->treeScope()->getElementById(element->fastGetAttribute(formAttr));
>	   if (formElement && formElement->hasTagName(formTag)) {
> -	       m_form = static_cast<HTMLFormElement*>(formElement);
> -	       m_form->registerFormElement(this);
> +	       newForm = static_cast<HTMLFormElement*>(formElement);
>	   }

Should remove braces here since the function body is now one line. I think we
could also consider renaming the local variable formElement, since it’s not
always a form element. Might call it something like newFormCandidate.

It is wasteful that this function calls both fastHasAttribute and
fastHasAttribute on the same attribute, doing a double hash table lookup. It
would be more efficient to only call fastGetAttribute and check for null
instead.

It would be good at some point to research if the behavior here is correct.
These are the two quirks I noticed:

- If a form attribute gives an ID that is used for an object other than a form
that comes earlier in the document than the form, then we don’t associate the
element with the form. Even if there is also a form with that ID.
- Changes to the ID of the form or of an object other than the form that comes
earlier in the document after the element is inserted into the tree have no
effect.

> Source/WebCore/html/FormAssociatedElement.h:73
> +    // If you add an override of willChangeForm() or didChangeForm() to a
> +    // subclass, you need to add setForm(0) to the destructor of the
subclass.

Would be better to use the formal C++ term, “class derived from this one” or
“derived class”, rather than “subclass”.

I’d say “If you add an override of willChangeForm() or didChangeForm() to a
class derived from this one, you will need to add a call to setForm(0) to the
destructor of that class".