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

Sun Apr 29 16:04:02 PDT 2012

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 139190: Updated patch
https://bugs.webkit.org/attachment.cgi?id=139190&action=review

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

> Source/WebCore/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

This line should appear before the description (i.e. lines 6 & 7) but still
surrounded by blank lines.

> Source/WebCore/html/RadioNodeList.cpp:80
> +	   if (!inputElement || inputElement->value() != value)
> +	       continue;
> +	   inputElement->setChecked(true);

This is going to make all radio button with the specified value checked, which
is not what the spec says.
You need to bail out from the loop after making the first element checked.
r- because of this bug.

> LayoutTests/fast/forms/form-collection-radio-node-list.html:6
> +<head>
> +<meta charset="utf-8">
> +<script src="../js/resources/js-test-pre.js"></script>
> +</head>

We don't need head for this test. You can just move the script element to the
body.