1452386 - Add a helper to get the existing nsIWeakReference from nsINode

Status: RESOLVED FIXED

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

Description

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

With such helper we should be able to improve the performance of bug 1452074 a bit more.

Comment 1

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

Attachment: getExistingweak.diff

Moving nsNodeWeakReference to nsINode.h so that there can be inline
GetExistingWeakReference.

This makes CustomElementRegistry::UnregisterUnresolvedElement faster, though I'm still not happy with the performance of that method. But perhaps this is good enough for now.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965980 [details] [diff] [review]
getExistingweak.diff

Review of attachment 8965980 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with nits addressed at your will. Thanks!

::: dom/base/CustomElementRegistry.cpp
@@ +370,5 @@
> +    return;
> +  }
> +
> +#ifdef DEBUG
> +  nsWeakPtr weakPtr = do_GetWeakReference(aElement);

nit: Maybe wrap this on its own block, like:

#ifdef DEBUG
{
  nsWeakPtr ...;
  MOZ_ASSERT(..);
}
#endif

That way weakPtr is only in the scope of the assertion, and not of the rest of the function.

::: dom/base/FragmentOrElement.cpp
@@ +614,5 @@
>  
>  NS_IMPL_ISUPPORTS(nsNodeWeakReference,
>                    nsIWeakReference)
>  
> +nsNodeWeakReference::nsNodeWeakReference(nsINode* aNode)

Maybe move all this to nsINode.cpp now it exist and it's declared there?

::: dom/base/nsINode.h
@@ +50,5 @@
>  class nsINodeList;
>  class nsIPresShell;
>  class nsIPrincipal;
>  class nsIURI;
> +class nsIWeakReference;

No need for the forward declaration if you're including the header.

@@ +267,5 @@
>  
> +/**
> + * A class that implements nsIWeakReference
> + */
> +

I know it's pre-existing, but I'd nix this newline.

@@ +271,5 @@
> +
> +class nsNodeWeakReference final : public nsIWeakReference
> +{
> +public:
> +  explicit nsNodeWeakReference(nsINode* aNode);

You can define this inline after the declaration of nsINode if you prefer, though not sure whether it's worth it.

@@ +278,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  // nsIWeakReference
> +  NS_DECL_NSIWEAKREFERENCE
> +  virtual size_t SizeOfOnlyThis(mozilla::MallocSizeOf aMallocSizeOf) const override;

nit: Since you're moving this, you may want to remove the unneeded `virtual`.

Comment 3

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)

> @@ +271,5 @@
> > +
> > +class nsNodeWeakReference final : public nsIWeakReference
> > +{
> > +public:
> > +  explicit nsNodeWeakReference(nsINode* aNode);
> 
> You can define this inline after the declaration of nsINode if you prefer,
> though not sure whether it's worth it.
That didn't compile, because the code here doesn't know that nsINode inherits nsISupports

Comment 4

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

Attachment: getExistingweak_2.diff

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1c08b1abda
Add a helper to get the existing nsIWeakReference from nsINode, r=emilio

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ac1c08b1abda