[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





--- Comment #19 from Shinya Kawanaka <shinyak at chromium.org>  2012-01-31 18:02:05 PST ---
(In reply to comment #17)
> (From update of attachment 124705 [details])
> 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.

They were removed.

> 
> > Source/WebCore/dom/Element.cpp:950
> > +        attachChildrenIfNotAttached();
> 
> Why are we attaching children anyway here? Can you explain?

I've added comments in the patch.

We have to attach children. Some of children are attached in shadow tree (in content element), but there might be unattached children. So attach them here anyway.

> 
> > 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()

Done.

> 
> > 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?

Done.

> 
> > 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?

Done.

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