[webkit-reviews] review denied: [Bug 48980] Implement form validation message UI : [Attachment 78660] Patch 3 (follow recent shadow DOM changes)

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 48980: Implement form validation message UI
https://bugs.webkit.org/show_bug.cgi?id=48980

Attachment 78660: Patch 3 (follow recent shadow DOM changes)
https://bugs.webkit.org/attachment.cgi?id=78660&action=review

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

Yay, first clean-slate new shadow DOM implementation! Comments below:

> Source/WebCore/html/ValidationMessage.cpp:77
> +    m_bubbleMessage->removeAllChildren();

Why do you need this?

> Source/WebCore/html/ValidationMessage.cpp:118
> +    RenderObject* hostRenderer = host->renderer();
> +    ASSERT(hostRenderer);

This should not be necessary anymore.

> Source/WebCore/html/ValidationMessage.cpp:121


Be careful here. Until we have a better API to apply multiple shadows, you will
be clobbering whatever was there before. And most inputs will have something
there. Soo... we probably need to invent some way to _add_ a validation
message, not replace existing shadow DOM with it.

> Source/WebCore/html/ValidationMessage.cpp:127
> +    host->setShadowRoot(m_bubble);
> +    RefPtr<RenderStyle> style =
doc->styleSelector()->styleForElement(m_bubble.get(), hostRenderer->style(),
false);
> +   
m_bubble->setRenderer(m_bubble->createRenderer(hostRenderer->renderArena(),
style.get()));
> +    m_bubble->renderer()->setStyle(style.release());
> +    m_bubble->setAttached();
> +    m_bubble->setInDocument();
> +    hostRenderer->addChild(m_bubble->renderer());

You shouldn't need any of this anymore.

> Source/WebCore/html/ValidationMessage.cpp:133
> +    // FIXME: We need to use different qualified names for the following
three
> +    // elements. CSSStyleSelector has a problem that each of elements with
the
> +    // same hierarchy and different pseudo IDs uses the style of the pseudo
ID
> +    // for the first element.

This sounds like a bug we should fix early on, not work around it.

> Source/WebCore/html/ValidationMessage.cpp:158
> +	   m_bubble->detach();

Shouldn't need this anymore.