[webkit-reviews] review denied: [Bug 90866] Add User Agent Shadow DOM to FormControlElement just before adding User Shadow DOM. : [Attachment 152153] Patch

Mon Jul 16 19:19:23 PDT 2012

Hajime Morrita <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 90866: Add User Agent Shadow DOM to FormControlElement just before adding
User Shadow DOM.
https://bugs.webkit.org/show_bug.cgi?id=90866

Attachment 152153: Patch
https://bugs.webkit.org/attachment.cgi?id=152153&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=152153&action=review

> Source/WebCore/ChangeLog:8
> +	   This patch ensures that UserAgentShadowDOM is the oldest ShadowDOM.

Nit: Usually the description mentions it change instead of patch.

> Source/WebCore/ChangeLog:14
> +	   If ValidationMessage does not assume the UserAgentShadowDOM is the
oldest, we have to make it the oldest,

Got confused. Does ValidationMessage assume it or not?

> Source/WebCore/ChangeLog:19
> +	   HTMLFormControlElement are derived from FormAssociatedElement. So we
will a callback to add

we will what?

> Source/WebCore/html/HTMLFormControlElement.cpp:72
> +ShadowRoot* HTMLFormControlElement::ensureUserAgentShadowRoot()

Similar code like this start scattering around here and there. 
Could you factor them out to kill such duplication?

>
LayoutTests/fast/dom/shadow/shadowdom-for-form-associated-element-with-validati
on.html:14
> +var shadowRoot = new WebKitShadowRoot(input);

I don't think checking crash is sufficient. How about to turn this into a
reftest?

> LayoutTests/fast/dom/shadow/shadowdom-for-form-associated-element.html:1
> +<!DOCTYPE html>

Ditto.

> LayoutTests/fast/dom/shadow/shadowdom-for-select-crash.html:1
> +<!DOCTYPE html>

Ditto.