[webkit-reviews] review denied: [Bug 57813] Support a list of nodes at the top level of a shadow DOM tree : [Attachment 88572] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Dominic Cooney
<dominicc at google.com>'s request for review:
Bug 57813: Support a list of nodes at the top level of a shadow DOM tree
https://bugs.webkit.org/show_bug.cgi?id=57813

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

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

I wonder if to minimize impact, the first patch should retain shadowRoot as
Node* and internalize creation and managing of ShadowRoot inside setShadowRoot?


We need to look at SVGShadowTreeRootElement and consider converting it to
ShadowRoot. Not right now, but as some next step.

> Source/WebCore/dom/Element.cpp:1053
> +    bool hasParentStyle = parentNodeForRendering() ?
parentNodeForRendering()->renderStyle() : false;

Because it's here, I wonder if -ForRendering is the wrong suffix. It's more
like ForRenderingAndStyle.

> Source/WebCore/dom/Element.cpp:1145
> +Node* Element::shadowRoot() const

Return ShadowRoot*?

> Source/WebCore/dom/Element.cpp:1150
>  void Element::setShadowRoot(PassRefPtr<Node> node)

Can this just take ShadowRoot?

> Source/WebCore/dom/Node.cpp:1425
> +	   || (!parentRenderer->canHaveChildren()
> +	       && !(isShadowRoot() || parentNode()->isShadowBoundary()))

Keep this one one line.

> Source/WebCore/dom/Node.h:206
> +    // FIXME: remove this when all shadow roots are ShadowRoots

Period at the end of a sentence.

> Source/WebCore/html/HTMLKeygenElement.cpp:81
> +    shadow->parserAddChild(select);

We are not the parser. We shouldn't be calling this. Use appendChild.

> Source/WebCore/html/HTMLMeterElement.cpp:235
> +    shadow->parserAddChild(bar);

Ditto.

> Source/WebCore/html/HTMLProgressElement.cpp:141
> +    bar->parserAddChild(m_value);

Ditto.

> Source/WebCore/html/HTMLProgressElement.cpp:143
> +    shadow->parserAddChild(bar);

Ditto.

> Source/WebCore/html/RangeInputType.cpp:201
> +   
shadow->parserAddChild(SliderThumbElement::create(element()->document()));

Use appendChild.

> Source/WebCore/rendering/RenderSlider.cpp:181
> +    return
toSliderThumbElement(static_cast<Element*>(node())->shadowRoot()->firstChild())
;

Probably need to check for shadowRoot existence here first.

> Source/WebKit/mac/DOM/WebDOMOperations.mm:48
> +#import <WebCore/ShadowFragment.h>

ShadowRoot?

> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:43
> +#include <WebCore/ShadowFragment.h>

ShadowRoot?