[webkit-reviews] review granted: [Bug 48980] Implement form validation message UI : [Attachment 78886] Patch 4

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 13 21:21:39 PST 2011


Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted 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 78886: Patch 4
https://bugs.webkit.org/attachment.cgi?id=78886&action=review

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

Let's give it a spin. I am not sure yet whether the design with the holder
class (ValidationMessage) is what we should use, but I don't know for sure. And
please appease the style elf. He's angry.

> Source/WebCore/css/CSSStyleSelector.cpp:984
> +	       (s->shadowPseudoId() == m_element->shadowPseudoId()) &&

Yay! Awesome.

> Source/WebCore/dom/Node.cpp:1384
> +    if (parentRenderer && (parentRenderer->canHaveChildren() ||
isShadowRoot()) && parent->childShouldCreateRenderer(this)) {

This is wrong, because we do need to respect canHaveChildren() on the renderer.
Or do we? This is an interesting question. Can you at least put a FIXME at the
top and file a bug to investigate this. So far, this is still a hack.

> Source/WebCore/html/ValidationMessage.cpp:120
> +    // FIXME: We need a way to host multiple shadow roots in a single node.

In XBL2, this is solved with inheritance, not multiple shadow roots. But yeah,
this looks like a hack.

> Source/WebCore/html/ValidationMessage.cpp:125
> +	   m_bubble->attach();

Shouldn't need this anymore. All attachment/detachment is done through the
standard means. Perhaps we could just setNeedsStyleRecalc or something like
that? At least put a FIXME here to investigate.


More information about the webkit-reviews mailing list