[webkit-reviews] review denied: [Bug 45719] Adding "checked" radio element to DOM, isn't checked : [Attachment 76478] proposed patch(2nd)

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied My Shin
<my.shin at obigo.com>'s request for review:
Bug 45719: Adding "checked" radio element to DOM, isn't checked
https://bugs.webkit.org/show_bug.cgi?id=45719

Attachment 76478: proposed patch(2nd)
https://bugs.webkit.org/attachment.cgi?id=76478&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76478&action=review

Also needs to be rebased for after the big Source move.

> WebCore/html/HTMLInputElement.cpp:685
> +	   if (renderer())

This is wrong. You can also have a NULL renderer() if you set display:none.
eseidel suggested earlier that you're probably looking for inDocument:
http://www.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/WebCore/d
om/Node.h&l=361&exact_package=chromium

> LayoutTests/fast/forms/radio-checked-by-javascript.html:31
> +    document.write("This should be checked");

document.write? ew.

> LayoutTests/fast/forms/radio-checked-by-javascript.html:39
> +var radioElement1, radioElement2, radioElement3, radioElement4;

no need to declare this outside of the function. Could just do var el =
document.getElementById('foo');

> LayoutTests/fast/forms/radio-checked-by-javascript.html:41
> +    radioElement1 = document.getElementById('radioID1');

Can you make this into a helper function that gets an element and verifies if
the element is checked? This way you would have a much more elegant code.