Drop makeWeakPtr() and use WeakPtr { } directly.
Created attachment 441093 [details] Patch
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
Created attachment 441095 [details] Patch
Created attachment 441096 [details] Patch
Created attachment 441128 [details] Patch
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?
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.
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.
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.
Created attachment 441155 [details] Patch
(In reply to Chris Dumez from comment #10) > Created attachment 441155 [details] > Patch I made the 2 suggested changes as they were painless.
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].
<rdar://problem/84227975>
Follow-up fix in https://commits.webkit.org/r284160 to address some assertions on Mac-wk1 debug bots.