[webkit-reviews] review denied: [Bug 76611] Content element should be able to be dynamically added/removed/replaced in a shadow tree. : [Attachment 124705] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 76611: Content element should be able to be dynamically
added/removed/replaced in a shadow tree.
https://bugs.webkit.org/show_bug.cgi?id=76611

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

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


> Source/WebCore/ChangeLog:19
> +	     Attaches only child elements if they are not attached.

"only" is in the wrong place here. Should be "Attaches child elements only if
they are not attached".

> Source/WebCore/ChangeLog:21
> +	     Detaches only child elements if they are attached.

ditto.

> Source/WebCore/dom/Element.cpp:950
> +	   attachChildrenIfNotAttached();

Why are we attaching children anyway here? Can you explain?

> Source/WebCore/dom/Element.cpp:976
> +void Element::attachChildrenIfNotAttached()
> +{
> +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> +	   if (!child->attached())
> +	       child->attach();
> +    }
> +}

Unless we still need this callsite above, we can probably fold this into
reattachChildrenAndShadow()

> Source/WebCore/dom/Element.cpp:998
> +void Element::detachChildrenIfAttached()
> +{
> +    for (Node* child = firstChild(); child; child = child->nextSibling()) {
> +	   if (child->attached())
> +	       child->detach();
> +    }
> +}

Ditto. While it's nice to break things up into the functions, the API surface
of Element is already ginormous. Perhaps a static local function?

> Source/WebCore/dom/ShadowRoot.cpp:104
> +	       host()->reattachChildrenAndShadow();

reattachChildrenAndShadow() has one callsite and when invoked, reaches back to
grab its callee. This seems weird. Can we just unroll this here? Or perhaps
make it a local function?