Bug 50971 – Combine setShadowRoot and clearShadowRoot into a simpler API

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

There's no need to have clearShadowHost, setShadowHost(0) should just do it, since it always needs to clean up previous value anyway.

[Dimitri Glazkov (Google)]

Created attachment 76446 [details]
Patch

[Darin Adler]

Comment on attachment 76446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76446&action=review

> WebCore/ChangeLog:5
> +        Combine setShadowHost and clearShadowHost into a simpler API

You mean setShadowRoot and clearShadowRoot, right?

> WebCore/dom/Element.cpp:1093
> +    } else if (!newRoot)

I think this else is wrong.

> WebCore/dom/Element.cpp:1097
> +    newRoot->setShadowHost(this);

If hasRareData was true and newRoot is a null pointer, then we’ll dereference the null pointer here.

> WebCore/dom/Node.h:220
> +    Element* shadowHost() const;
> +    void setShadowHost(Element*);

Why are you making these member functions public? Seems unrelated to the rest of the patch?

[Dimitri Glazkov (Google)]

Comment on attachment 76446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76446&action=review

Thanks for the review! New patch coming up.

>> WebCore/dom/Element.cpp:1093
>> +    } else if (!newRoot)
> 
> I think this else is wrong.

You're right! I don't know what I've been drinking last night.

>> WebCore/dom/Element.cpp:1097
>> +    newRoot->setShadowHost(this);
> 
> If hasRareData was true and newRoot is a null pointer, then we’ll dereference the null pointer here.

Yup. My "else" above was enabling this to happen.

>> WebCore/dom/Node.h:220
>> +    void setShadowHost(Element*);
> 
> Why are you making these member functions public? Seems unrelated to the rest of the patch?

Because I need to use setShadowHost() from Element::setShadowRoot() and thought I might as well bring shadowHost() out with it.

[Dimitri Glazkov (Google)]

Created attachment 76548 [details]
Now with less confusion.

[Eric Seidel (no email)]

Comment on attachment 76548 [details]
Now with less confusion.

View in context: https://bugs.webkit.org/attachment.cgi?id=76548&action=review

> WebCore/dom/Element.cpp:1093
> +        if (RefPtr<Node> oldRoot = rareData()->m_shadowRoot.release()) {
> +            document()->removeFocusedNodeOfSubtree(oldRoot.get());
> +            oldRoot->setShadowHost(0);
> +            if (oldRoot->inDocument())
> +                oldRoot->removedFromDocument();
> +            else
> +                oldRoot->removedFromTree(true);

This feels like a helper method.

[Dimitri Glazkov (Google)]

Created attachment 76776 [details]
With more clarity in code.

[Eric Seidel (no email)]

Comment on attachment 76776 [details]
With more clarity in code.

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.

[Dimitri Glazkov (Google)]

Comment on attachment 76776 [details]
With more clarity in code.

View in context: https://bugs.webkit.org/attachment.cgi?id=76776&action=review

Cq to the resQUE! :)

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

Yeah, I could've danced around this with just using node.get() when assigning m_shadowRoot to avoid PassRefPtr derefing. But I generally don't like using PassRefPtrs for doing anything other than passing :)

>> WebCore/dom/Element.cpp:1093
>> +    ensureRareData()->m_shadowRoot = newRoot;
> 
> You could just assign it here w/o the check, no?

No, because ensureRareData always creates a new ElementRareData, which is not necessary if newRoot is 0.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76776 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 76776 [details]
With more clarity in code.

Clearing flags on attachment: 76776

Committed r74715: <http://trac.webkit.org/changeset/74715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.