[Webkit-unassigned] [Bug 50971] Combine setShadowRoot and clearShadowRoot into a simpler API

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76776|review?                     |review+
               Flag|                            |




--- Comment #7 from Eric Seidel <eric at webkit.org>  2010-12-24 09:05:48 PST ---
(From update of attachment 76776)
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.

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