Bug 216945 – Get rid of AudioNode::RefType

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Get rid of AudioNode::RefType. Try to make AudioNode refcounting clearer and less leak-prone.

[Chris Dumez]

Created attachment 409642 [details]
Patch

[Chris Dumez]

Created attachment 409645 [details]
Patch

[Chris Dumez]

Created attachment 409647 [details]
Patch

[Chris Dumez]

Created attachment 409648 [details]
Patch

[Chris Dumez]

Any thoughts on this?

[Darin Adler]

Comment on attachment 409648 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409648&action=review

> Source/WTF/ChangeLog:11
> +        Add third template parameter to RefPtr allowing to define the traits
> +        from incrementing / decrementing the refcount. The default traits
> +        call ref() / deref() but this can now be customized to call other
> +        functions.

What about all the other RefPtr-like classes? Could RetainPtr be defined like this, or does it have features that are too fancy? WKRetainPtr? JSRetainPtr? NakedPtr? OSObjectPtr?

> Source/WTF/wtf/RefPtr.h:49
> +template<typename T, typename _PtrTraits, typename _RefDerefTraits>

Why the leading underscores? I think that’s technically reserved for the standard library.

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 409648 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409648&action=review
> 
> > Source/WTF/ChangeLog:11
> > +        Add third template parameter to RefPtr allowing to define the traits
> > +        from incrementing / decrementing the refcount. The default traits
> > +        call ref() / deref() but this can now be customized to call other
> > +        functions.
> 
> What about all the other RefPtr-like classes? Could RetainPtr be defined
> like this, or does it have features that are too fancy? WKRetainPtr?
> JSRetainPtr? NakedPtr? OSObjectPtr?

We don't have such need for the other classes yet. I had initially written my own class to wrap an AudioNode and call incrementConnectionCount() in constructor and decrementConnectionCount() in the destructor. I eventually realized I was duplicating a lot of the RefPtr code so I decided to make RefPtr more flexible so that I can reuse it instead.

If you don't like this approach though, I can try and go back to my original idea. I do want the calls to incrementConnectionCount() / decrementConnectionCount() to be abstracted away though to reduce the chance of leaks.

> > Source/WTF/wtf/RefPtr.h:49
> > +template<typename T, typename _PtrTraits, typename _RefDerefTraits>
> 
> Why the leading underscores? I think that’s technically reserved for the
> standard library.

I could not do:
using PtrTraits = PtrTraits;

So I needed a way to differentiate the template parameter. I decided to use an underscore but I am open to suggestions.

[EWS]

Committed r267591: <https://trac.webkit.org/changeset/267591>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409648 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69591533>

[Myles C. Maxfield]

By the way, it looks like this has broken binary compatibility with open-source builds (because those builds use the system WebInspector.framework).

[Chris Dumez]

(In reply to Myles C. Maxfield from comment #10)
> By the way, it looks like this has broken binary compatibility with
> open-source builds (because those builds use the system
> WebInspector.framework).

I am unclear what this change has to do with WebInspector.framework? I do not see any changes to inspector bits in this patch.

[Darin Adler]

I’m guessing it’s the change to RefPtr template arguments.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> I’m guessing it’s the change to RefPtr template arguments.

Oh, that makes sense indeed.

[Darin Adler]

I don’t understand how mismatched WebKit and WebInspector frameworks is expected to work. It’s untenable to have such a fragile API contract between them; not sure when we decided on this strategy.

[Myles C. Maxfield]

See also: https://bugs.webkit.org/show_bug.cgi?id=217778 and https://bugs.webkit.org/show_bug.cgi?id=215961