[webkit-reviews] review granted: [Bug 50971] Combine setShadowRoot and clearShadowRoot into a simpler API : [Attachment 76776] With more clarity in code.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 24 09:05:48 PST 2010
Eric Seidel <eric at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 50971: Combine setShadowRoot and clearShadowRoot into a simpler API
https://bugs.webkit.org/show_bug.cgi?id=50971
Attachment 76776: With more clarity in code.
https://bugs.webkit.org/attachment.cgi?id=76776&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76776&action=review
Seems OK to me.
> WebCore/dom/Element.cpp:1087
> + // FIXME: Because today this is never called from script directly, we
don't have to worry
> + // about compromising DOM tree integrity (eg. node being a parent of
this). However,
> + // once we implement XBL2, we will have to add integrity checks here.
Can we ASSERT here instead of just a FIXME? I'm not sure what we'd ASSERT
though...
> WebCore/dom/Element.cpp:1089
> + RefPtr<Node> newRoot = node;
I'm not sure why you use the newRoot here. We don't really need the extra ref.
Maybe Darin asked you to use a local RefPtr?
> WebCore/dom/Element.cpp:1093
> + ensureRareData()->m_shadowRoot = newRoot;
You could just assign it here w/o the check, no?
> WebCore/dom/Element.cpp:1094
> + newRoot->setShadowHost(this);
Then you would check ensureRareData()->m_shadowRoot before calling
ensureRareData()->m_shadowRoot->setShadowHost(this)?
Donno. Your current setup is fine too.
More information about the webkit-reviews
mailing list