[webkit-reviews] review denied: [Bug 81854] RadioNodeList support in HTMLFormElement::elements : [Attachment 136420] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 81854: RadioNodeList support in HTMLFormElement::elements
https://bugs.webkit.org/show_bug.cgi?id=81854

Attachment 136420: Updated patch
https://bugs.webkit.org/attachment.cgi?id=136420&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136420&action=review


> Source/WebCore/html/RadioNodeList.cpp:39
> +    : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ?
forNode->document() : forNode)

When can forNode not be formTag? As far as I know, HTMLFormElement::elements is
the only function that creates HTMLFormCollection.
It's better to assert that forNode is indeed a HTML form element. Also, please
rename m_forNode to m_formElement.
r- because of this.

> Source/WebCore/html/RadioNodeList.cpp:56
> +	   if (!node->isElementNode())
> +	       continue;

This continue will never be executed because nodeMatches returns false for
non-element nodes.
It's better to assert in this case.

> Source/WebCore/html/RadioNodeList.cpp:79
> +	   Element* element = toElement(node);
> +	   if (!element->hasTagName(inputTag))
> +	       continue;
> +	   HTMLInputElement* inputElement =
static_cast<HTMLInputElement*>(element);
> +	   if (!inputElement->isRadioButton() ||
inputElement->value().isEmpty() || inputElement->value() != val)
> +	       continue;

We're repeating this logic here. Please extract it as a helper function.

> LayoutTests/fast/forms/form-collection-radio-node-list.html:16
> +container.innerHTML = '<form id="form1">' +
> +    '<button id=button1></button>' +

You said there are cases RadioNodeList needs to return an element that's not
necessarily in the subtree for form.
If that's really the case, then we definitely need a test case for that.
But looking at your code and also the spec, I don't see how that's possible.