Closed Bug 1439223 Opened 2 years ago Closed 2 years ago

ShadowRoot::Host does unnecessary function calls for nothing.

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(1 file)

No description provided.
Priority: -- → P2
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)
Attachment #8951980 - Flags: review?(bugs) → review+
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!
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2931f4f700ef
Stop ShadowRoot::Host from being a useless function call. r=smaug
https://hg.mozilla.org/mozilla-central/rev/2931f4f700ef
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.