Bug 216894 – Use less explicit ref() / deref() calls in WebAudio code

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Use less explicit ref() / deref() calls in WebAudio code.

[Chris Dumez]

Created attachment 409498 [details]
Patch

[Darin Adler]

Comment on attachment 409498 [details]
Patch

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

> Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp:88
> +    auto protectedThis = makeRef(*this);

Not sure I understand where the need for this comes from exactly. Not that I am arguing to not do it, just that I was wondering how I would have known it’s needed.

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:102
> +    m_renderThread = Thread::create("offline renderer", [this, protectedThis = WTFMove(protectedThis)]() mutable {

Not sure anyone else agrees, but I’ve always been a fan of *not* capturing this in cases like this. I know it makes the code less elegant.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 409498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409498&action=review
> 
> > Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp:88
> > +    auto protectedThis = makeRef(*this);
> 
> Not sure I understand where the need for this comes from exactly. Not that I
> am arguing to not do it, just that I was wondering how I would have known
> it’s needed.

The pre-existing code was already ref-ing this but from inside the lock() function (and deref'ing in unlock()). I added the protector to maintain pre-existing behavior.

> 
> > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:102
> > +    m_renderThread = Thread::create("offline renderer", [this, protectedThis = WTFMove(protectedThis)]() mutable {
> 
> Not sure anyone else agrees, but I’ve always been a fan of *not* capturing
> this in cases like this. I know it makes the code less elegant.

[EWS]

Committed r267505: <https://trac.webkit.org/changeset/267505>

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

[Radar WebKit Bug Importer]

<rdar://problem/69463911>