[webkit-reviews] review denied: [Bug 78878] Adding a ShadowRoot to image-backed element causes a crash : [Attachment 127566] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 78878: Adding a ShadowRoot to image-backed element causes a crash
https://bugs.webkit.org/show_bug.cgi?id=78878

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

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


This is interesting. This specific bit of code uses CSS content property, which
means that not rendering children is a purely CSS concept, and not something we
could ever rationalize in terms of the shadow DOM. These two parallel worlds
(CSS replaced content and shadow DOM) will continue to need to coexist.

In this sense, the meaning of canHaveChildren changes to "will the renderers
ever be created for my children" and "will I have children renderers attached
to my renderer as children". I think we should try to stick to these concepts
and decouple them from the shadow DOM itself. Does it make sense?

> Source/WebCore/dom/NodeRenderingContext.cpp:279
> +	   if (RenderObject::ParentableForAll !=
parentRenderer->parentability())

It's weird, but you never check for ParentableForShadow. If there are only two
states, why have more than two exposed?

> Source/WebCore/dom/NodeRenderingContext.cpp:284
> +	   ASSERT(m_phase == AttachingShadowChild

Now that I look at it, "ShadowChild" doesn't make sense anymore. I think we
should align this with the spec wording, too.

> Source/WebCore/rendering/RenderButton.h:59
> +    virtual Parentability parentability() const OVERRIDE;

parentability is not a real word :)