Bug 216945 - Get rid of AudioNode::RefType
Summary: Get rid of AudioNode::RefType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-09-24 15:12 PDT by Chris Dumez
Modified: 2021-01-13 17:29 PST (History)
17 users (show)

See Also:


Attachments
Patch (41.99 KB, patch)
2020-09-24 17:45 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.97 KB, patch)
2020-09-24 18:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.06 KB, patch)
2020-09-24 18:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.89 KB, patch)
2020-09-24 18:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-09-24 15:12:43 PDT
Get rid of AudioNode::RefType. Try to make AudioNode refcounting clearer and less leak-prone.
Comment 1 Chris Dumez 2020-09-24 17:45:14 PDT
Created attachment 409642 [details]
Patch
Comment 2 Chris Dumez 2020-09-24 18:09:02 PDT
Created attachment 409645 [details]
Patch
Comment 3 Chris Dumez 2020-09-24 18:37:12 PDT
Created attachment 409647 [details]
Patch
Comment 4 Chris Dumez 2020-09-24 18:49:47 PDT
Created attachment 409648 [details]
Patch
Comment 5 Chris Dumez 2020-09-25 12:05:56 PDT
Any thoughts on this?
Comment 6 Darin Adler 2020-09-25 12:15:22 PDT
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.
Comment 7 Chris Dumez 2020-09-25 12:20:12 PDT
(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.
Comment 8 EWS 2020-09-25 13:36:55 PDT
Committed r267591: <https://trac.webkit.org/changeset/267591>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409648 [details].
Comment 9 Radar WebKit Bug Importer 2020-09-25 13:37:21 PDT
<rdar://problem/69591533>
Comment 10 Myles C. Maxfield 2021-01-13 10:30:55 PST
By the way, it looks like this has broken binary compatibility with open-source builds (because those builds use the system WebInspector.framework).
Comment 11 Chris Dumez 2021-01-13 10:34:41 PST
(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.
Comment 12 Darin Adler 2021-01-13 10:35:10 PST
I’m guessing it’s the change to RefPtr template arguments.
Comment 13 Chris Dumez 2021-01-13 10:35:44 PST
(In reply to Darin Adler from comment #12)
> I’m guessing it’s the change to RefPtr template arguments.

Oh, that makes sense indeed.
Comment 14 Darin Adler 2021-01-13 10:36:16 PST
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.