[Webkit-unassigned] [Bug 76611] Content element should be able to be dynamically added/removed/replaced in a shadow tree.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76611


Dimitri Glazkov (Google) <dglazkov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #124705|review?                     |review-
               Flag|                            |




--- Comment #17 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2012-01-31 10:13:28 PST ---
(From update of attachment 124705)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.