1439223 - ShadowRoot::Host does unnecessary function calls for nothing.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439223: Stop ShadowRoot::Host from being a useless function call.

Also, change DocumentFragment to use RefPtr, since that's the usual style.

Review commit: https://reviewboard.mozilla.org/r/221226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221226/

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8951980 [details]
Bug 1439223: Stop ShadowRoot::Host from being a useless function call.

https://reviewboard.mozilla.org/r/221226/#review227446

::: dom/base/DocumentFragment.h:110
(Diff revision 1)
>    {
>    }
>  
>    nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult,
>                   bool aPreallocateChildren) const override;
> -  nsCOMPtr<Element> mHost;
> +  RefPtr<Element> mHost;

FWIW, I tend to try to use nsCOMPtr with Element, just because Element has IID and nsCOMPtr adds some debug only safety checks. But in practice doesn't matter much.

::: dom/base/ShadowRoot.h:70
(Diff revision 1)
>               nsXBLPrototypeBinding* aProtoBinding);
>  
>    // Shadow DOM v1
> -  Element* Host();
> +  Element* Host() const
> +  {
> +    MOZ_ASSERT(GetHost(), "ShadowRoot always has a host, how did we create "

The assertion, which you just moved, is technically wrong. After unlink GetHost will return null.
But I guess keeping the assertion is ok, since if it fires, someone is using Host() are wrong time.

(I wish we had a way to expose some methods to bindings only, like Host(), which shouldn't return ever null when there is still JS pointing to the ShadowRoot)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8951980 [details]
Bug 1439223: Stop ShadowRoot::Host from being a useless function call.

https://reviewboard.mozilla.org/r/221226/#review227446

> FWIW, I tend to try to use nsCOMPtr with Element, just because Element has IID and nsCOMPtr adds some debug only safety checks. But in practice doesn't matter much.

I thought nsCOMPtr was meant to be used for nsIThings. I've definitely received review comments telling me to stop using nsCOMPtr for element... Huh.

There's a more subtle difference I think, and it's that nsCOMPtr's refcounting is virtual, unlike RefPtr's... It's not a huge difference anyway, but worth noting it I guess.

> The assertion, which you just moved, is technically wrong. After unlink GetHost will return null.
> But I guess keeping the assertion is ok, since if it fires, someone is using Host() are wrong time.
> 
> (I wish we had a way to expose some methods to bindings only, like Host(), which shouldn't return ever null when there is still JS pointing to the ShadowRoot)

I see... That's slightly annoying indeed. Thanks for the review!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2931f4f700ef
Stop ShadowRoot::Host from being a useless function call. r=smaug

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2931f4f700ef