Bug 231679 – Drop makeWeakPtr() and use WeakPtr { } directly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Drop makeWeakPtr() and use WeakPtr { } directly.

[Chris Dumez]

Created attachment 441093 [details]
Patch

[EWS Watchlist]

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]

Created attachment 441095 [details]
Patch

[Chris Dumez]

Created attachment 441096 [details]
Patch

[Chris Dumez]

Created attachment 441128 [details]
Patch

[Darin Adler]

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 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 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 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]

Created attachment 441155 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> Created attachment 441155 [details]
> Patch

I made the 2 suggested changes as they were painless.

[EWS]

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]

<rdar://problem/84227975>

[Chris Dumez]

Follow-up fix in https://commits.webkit.org/r284160 to address some assertions on Mac-wk1 debug bots.