882561 - Add GenericRefCounted using virtual instead of templates for polymorphism, like nsISupports

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Add GenericRefCounted

Use case: gfx/2d needs a generic non-templated refcounting base (to accept a generic "extension field" in some non-templated class) and does not want to have a dependency on XPCOM (aims to be shareable with non-mozilla code and buildable stand-alone).

This works with the existing RefPtr<T> pointer.

It's even possible to generic RefPtr<GenericRefCounted> pointers, though of course one then needs to manually static_cast the result of .get() to get something useful. The more typical use case is to use type-specific RefPtr<T> with T inheriting GenericRefCounted.

Is this OK to have in MFBT?

Unit test coming.

Comment 1

[(no longer active)]

Firstly, this is a terrible name, as it makes it sound like GenericRefCounted is better than RefCounted somehow.

But if this is related to the discussion of refcounting the way that graphics code does it, I'd like to keep this terrible-ness out of MFBT to discourage its use.  Why can't this live in gfx/2d?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

I don't feel like fighting for this until uses cases outside of gfx/2d emerge, so I'm OK to put this in gfx/2d instead for now.

One thing though, having this in MFBT enabled the unit test I'm adding in bug 882562 to cover this as well.

Still interested in Jeff's thoughts on this.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Another kind of use cases is when you want to be able to AddRef/Release something without #include<something.h>. Just mentioning.

It was also unintentional that the name would "sound like GenericRefCounted is better than RefCounted somehow" --- definitely not the intention. I thought of it as just a variant that might be useful in some cases.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

In an off-line conversation with Ehsan we agreed to let this sit in gfx/2d for now and possibly revisit if other use cases appear.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 761884 [details] [diff] [review]
Add GenericRefCounted

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

I'm fine enough with this in gfx/2d, certainly, in theory.  But if we do that, doesn't that mean the refcounting bits will have to be implemented elsewhere too?  I'd really rather the reference-counting code not be spread across too many places -- we should have it all in one place.  Given that, I think I slightly lean toward having this here even if it's slightly uglifying that way.  But it's up to you, you're the one adding this.

Regarding the name and its suggesting betterness than RefCounted, maybe RefCountedVirtual would be a name with less unthinking appeal?  I dunno.

::: mfbt/RefPtr.h
@@ +132,5 @@
> + */
> +class GenericRefCountedBase
> +{
> +  protected:
> +    virtual ~GenericRefCountedBase() {};

Pretty sure the extra ; here will make some compilers cry.

Comment 6

[(no longer active)]

(In reply to comment #5)
> I'm fine enough with this in gfx/2d, certainly, in theory.  But if we do that,
> doesn't that mean the refcounting bits will have to be implemented elsewhere
> too?  I'd really rather the reference-counting code not be spread across too
> many places -- we should have it all in one place.  Given that, I think I
> slightly lean toward having this here even if it's slightly uglifying that way.
>  But it's up to you, you're the one adding this.

gfx/2d is the only use case, and the reason why it needs this is pretty edge-casey, so hopefully it will remain the only user.  We can always move this if others need the same idiom.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 761884 [details] [diff] [review]
Add GenericRefCounted

gw280 wanted an r+ on putting this in gfx/2d, so there.  :-)  Just don't be touching mozilla::detail as this current patch does (and as I'd expect a translated patch would not), and I'm fine with this.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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