[webkit-reviews] review canceled: [Bug 183483] RealtimeOutgoingAudioSource and RealtimeOutgoingVideoSource should be destroyed on the main thread : [Attachment 335359] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has canceled youenn fablet
<youennf at gmail.com>'s request for review:
Bug 183483: RealtimeOutgoingAudioSource and RealtimeOutgoingVideoSource should
be destroyed on the main thread
https://bugs.webkit.org/show_bug.cgi?id=183483

Attachment 335359: Patch

https://bugs.webkit.org/attachment.cgi?id=335359&action=review




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 335359
  --> https://bugs.webkit.org/attachment.cgi?id=335359
Patch

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

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:79
>> +	RefPtr<Thread> m_thread;
> 
> Is this OK? Are we sure this does not create a cycle?

It should not create a cycle, since the thread entry function is deleted after
being called.
That said, looking at it further, there is probably no need for ThreadKeeper at
all.

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:84
>> +	// Free senders in a different thread like could be done with actual
peer connection.
> 
> So we can just free senders on any thread but not the current one?

Since it is a mock, this would always be freed in the main thread.
And we want to test the case of sources being derefed in a non main thread.

>> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:86
>> +	auto threadFunction =  [this, keeper = keeper.copyRef(), senders =
WTFMove(m_senders)] { };
> 
> Why is this capture this? Any use of this would likely crash.

Not needed indeed.