Closed Bug 1452386 Opened 2 years ago Closed 2 years ago

Add a helper to get the existing nsIWeakReference from nsINode

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

With such helper we should be able to improve the performance of bug 1452074 a bit more.
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.
Attachment #8965980 - Flags: review?(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`.
Attachment #8965980 - Flags: review?(emilio) → review+
(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
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1c08b1abda
Add a helper to get the existing nsIWeakReference from nsINode, r=emilio
https://hg.mozilla.org/mozilla-central/rev/ac1c08b1abda
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.