[Webkit-unassigned] [Bug 57813] Support a list of nodes at the top level of a shadow DOM tree

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Dominic Cooney <dominicc at google.com>  2011-04-07 10:43:45 PST ---
(In reply to comment #6)
> (From update of attachment 88572 [details])
> 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?

I could do this, but I like it this way because now internal shadows can start to use multiple children in shadows.

Also, if we make setShadowRoot() create the ShadowRoot, that breaks symmetry with shadowRoot(). We could make shadowRoot() do [ShadowRoot instance]->firstChild(), but it feels like back to square one.

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

+1

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

Done

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

I would like to, but per your comments on the first patch, the patch bloats a bit if I do this, because shadowRoot() callers need to #include "ShadowRoot.h" to grok ShadowRoot* is assignable to Node*. Happy to make it so… WDYT?

> > Source/WebCore/dom/Element.cpp:1150
> >  void Element::setShadowRoot(PassRefPtr<Node> node)
> 
> Can this just take ShadowRoot?

Done. Does it make sense to make shadowRoot return ShadowRoot* for symmetry?

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

Done.

> > Source/WebCore/dom/Node.h:206
> > +    // FIXME: remove this when all shadow roots are ShadowRoots
> 
> Period at the end of a sentence.

Done.

> > Source/WebCore/html/HTMLKeygenElement.cpp:81
> > +    shadow->parserAddChild(select);
> 
> We are not the parser. We shouldn't be calling this. Use appendChild.

Done.

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

Done.

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

Done.

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

Done.

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

Done.

> > Source/WebCore/rendering/RenderSlider.cpp:181
> > +    return toSliderThumbElement(static_cast<Element*>(node())->shadowRoot()->firstChild());
> 
> Probably need to check for shadowRoot existence here first.

Done. Added an assertion to HTMLKeygenElement::shadowSelect, since that should always exist.

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

My hair just turned white. I will do a clean build and rebuild the test shells.

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

Done.

New patch soon.

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