Summary: | Use less explicit ref() / deref() calls in WebAudio code | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | Web Audio | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | calvaris, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jer.noble, philipj, sergio, webkit-bug-importer, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 212611 | ||||||
Attachments: |
|
Description
Chris Dumez
2020-09-23 13:30:58 PDT
Created attachment 409498 [details]
Patch
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. (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. Committed r267505: <https://trac.webkit.org/changeset/267505> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409498 [details]. |