Bug 231679

Summary: Drop makeWeakPtr() and use WeakPtr { } directly
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web Template FrameworkAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, achristensen, andresg_22, apinheiro, benjamin, berto, cfleizach, cgarcia, changseok, cmarcelo, darin, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gustavo, gyuyoung.kim, hi, japhet, jbedard, jcraig, jdiggs, jer.noble, jiewen_tan, joepeck, kangil.han, mifenton, mkwst, pangle, philipj, samuel_white, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 231580    
Bug Blocks:    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Chris Dumez
Reported 2021-10-13 09:11:32 PDT
Drop makeWeakPtr() and use WeakPtr { } directly.
Attachments
Patch (205.48 KB, patch)
2021-10-13 09:48 PDT, Chris Dumez
ews-feeder: commit-queue-
Patch (205.49 KB, patch)
2021-10-13 10:08 PDT, Chris Dumez
ews-feeder: commit-queue-
Patch (206.38 KB, patch)
2021-10-13 10:27 PDT, Chris Dumez
ews-feeder: commit-queue-
Patch (215.81 KB, patch)
2021-10-13 13:37 PDT, Chris Dumez
no flags
Patch (216.93 KB, patch)
2021-10-13 16:05 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2021-10-13 09:48:10 PDT
EWS Watchlist
Comment 2 2021-10-13 09:49:00 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Chris Dumez
Comment 3 2021-10-13 10:08:08 PDT
Chris Dumez
Comment 4 2021-10-13 10:27:18 PDT
Chris Dumez
Comment 5 2021-10-13 13:37:14 PDT
Darin Adler
Comment 6 2021-10-13 15:17:57 PDT
Comment on attachment 441128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441128&action=review > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:233 > + gpuProcess.audioSessionManager().session().setRoutingArbitrationClient(WeakPtr { m_routingArbitrator.get() }); Can we leave out the explicit conversion to WeakPtr? I saw many other cases where it seems we were able to do that. > Source/WebKit/GPUProcess/media/RemoteCDMInstanceProxy.cpp:56 > + m_instance->setClient(static_cast<CDMInstanceClient&>(*this)); Why is the static_cast needed? What compilation failure do we get without the cast?
Chris Dumez
Comment 7 2021-10-13 15:20:55 PDT
Comment on attachment 441128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441128&action=review >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:233 >> + gpuProcess.audioSessionManager().session().setRoutingArbitrationClient(WeakPtr { m_routingArbitrator.get() }); > > Can we leave out the explicit conversion to WeakPtr? I saw many other cases where it seems we were able to do that. I had initially tried to drop the explicit conversion but it failed to build so I put it back. The issue is that setRoutingArbitrationClient() takes in a `WeakPtr<AudioSessionRoutingArbitrationClient>&&`, but AudioSessionRoutingArbitrationClient doesn't subclass CanMakeWeakPtr (its subclasses do). It is a bit weird it was implemented this way. I tried not to refactor the code too much in this patch.
Chris Dumez
Comment 8 2021-10-13 15:23:29 PDT
Comment on attachment 441128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441128&action=review >> Source/WebKit/GPUProcess/media/RemoteCDMInstanceProxy.cpp:56 >> + m_instance->setClient(static_cast<CDMInstanceClient&>(*this)); > > Why is the static_cast needed? What compilation failure do we get without the cast? The issue is that RemoteCDMInstanceProxy inherits CanMakeWeakPtr<> from 2 base classes, this cast resolves the ambiguity. We could also resolve the ambiguity with a couple of `using` statements in the RemoteCDMInstanceProxy header too though.
Darin Adler
Comment 9 2021-10-13 15:34:07 PDT
Comment on attachment 441128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441128&action=review >>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:233 >>> + gpuProcess.audioSessionManager().session().setRoutingArbitrationClient(WeakPtr { m_routingArbitrator.get() }); >> >> Can we leave out the explicit conversion to WeakPtr? I saw many other cases where it seems we were able to do that. > > I had initially tried to drop the explicit conversion but it failed to build so I put it back. > > The issue is that setRoutingArbitrationClient() takes in a `WeakPtr<AudioSessionRoutingArbitrationClient>&&`, but AudioSessionRoutingArbitrationClient doesn't subclass CanMakeWeakPtr (its subclasses do). It is a bit weird it was implemented this way. I tried not to refactor the code too much in this patch. I am kind of surprised that works; can’t really figure out how it does. How can WeakPtr<AudioSessionRoutingArbitrationClient> member functions successfully compile? I guess it’s down to the implementation internals of WeakPtr. After landing this, it seems like we should streamline this code by moving CanMakeWeakPtr into AudioSessionRoutingArbitrationClient unless that would lead to derived classes that inherit from two different CanMakeWeakPtr bases. >>> Source/WebKit/GPUProcess/media/RemoteCDMInstanceProxy.cpp:56 >>> + m_instance->setClient(static_cast<CDMInstanceClient&>(*this)); >> >> Why is the static_cast needed? What compilation failure do we get without the cast? > > The issue is that RemoteCDMInstanceProxy inherits CanMakeWeakPtr<> from 2 base classes, this cast resolves the ambiguity. We could also resolve the ambiguity with a couple of `using` statements in the RemoteCDMInstanceProxy header too though. I wouldn’t block landing the patch over this; on the other hand, I think the using statements would be a more elegant solution.
Chris Dumez
Comment 10 2021-10-13 16:05:21 PDT
Chris Dumez
Comment 11 2021-10-13 16:09:26 PDT
(In reply to Chris Dumez from comment #10) > Created attachment 441155 [details] > Patch I made the 2 suggested changes as they were painless.
EWS
Comment 12 2021-10-13 18:46:33 PDT
Committed r284142 (242962@main): <https://commits.webkit.org/242962@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441155 [details].
Radar WebKit Bug Importer
Comment 13 2021-10-13 18:47:20 PDT
Chris Dumez
Comment 14 2021-10-14 07:49:13 PDT
Follow-up fix in https://commits.webkit.org/r284160 to address some assertions on Mac-wk1 debug bots.
Note You need to log in before you can comment on or make changes to this bug.