[webkit-reviews] review granted: [Bug 220502] Radio buttons with no form owner are not grouped : [Attachment 446548] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 220502: Radio buttons with no form owner are not grouped
https://bugs.webkit.org/show_bug.cgi?id=220502

Attachment 446548: Patch

https://bugs.webkit.org/attachment.cgi?id=446548&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 446548
  --> https://bugs.webkit.org/attachment.cgi?id=446548
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446548&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1617
> +    if (removalType.disconnectedFromDocument && !form() && isRadioButton())
> +	   updateValidity();

Maybe it’s obvious, but why can we skip this update if form() is non-null?

> Source/WebCore/html/HTMLInputElement.cpp:1967
> +    if (checked())
> +	   return const_cast<HTMLInputElement*>(this);

I guess this is an optimization. Or is it for correctness?

> Source/WebCore/html/HTMLInputElement.cpp:1980
> +    // The input is not managed by a RadioButtonGroups, we'll need to
traverse the tree.
> +    for (auto& descendant : descendantsOfType<HTMLInputElement>(rootNode()))
{
> +	   if (descendant.isRadioButton() && name == descendant.name() &&
!descendant.form() && descendant.checked())
> +	       return &descendant;
> +    }

I understand that the Chromium engineer who wrote this code was OK with the
performance cost of walking the tree every time. But we are OK with that? What
is the pathological worst case version of this?

Also, the checked() function is a super-fast one, so maybe it should go earlier
in this clause. Faster than isRadioButton (which calls a virtual function),
faster than a string comparison, even faster than the really simple form
function call (which is not inlined).

> Source/WebCore/html/RadioInputType.cpp:49
> +    auto name = element()->name();

Maybe auto& to avoid some reference count churn.

> Source/WebCore/html/RadioInputType.cpp:59
> +    auto& treeRoot = element()->rootNode();
> +    for (auto* input =
Traversal<HTMLInputElement>::inclusiveFirstWithin(treeRoot); input; input =
Traversal<HTMLInputElement>::next(*input, &treeRoot)) {

I’d rather have this use inclusiveDescendantsOfType, iterator style. You’d have
to add that, though. It would go in TypedDescendantsIterator.h:

    for (auto& input :
inclusiveDescendantsOfType<HTMLInputElement>(element()->rootNode())) {

Same question as above about the cost of walking the tree every time.

> Source/WebCore/html/RadioInputType.cpp:67
> +	   if (isChecked && isRequired)
> +	       return false;

We can return false as soon as *isChecked* is true, doesn’t matter what the
state of *isRequired* is. So we don’t need an isChecked boolean, just return
false as soon as we find a checked one.