Bug 226672 – Reduce use of legacy MainThreadTaskQueue in media code

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

Reduce use of legacy MainThreadTaskQueue in media code.

[Chris Dumez]

Created attachment 430639 [details]
Patch

[Darin Adler]

Comment on attachment 430639 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/TextTrackRepresentationCocoa.mm:155
> +        if (weakThis)
> +            weakThis->client().textTrackRepresentationBoundsChanged(weakThis->bounds());

Is bounds() guaranteed to not do any work that could have the side effect of destroying this?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:182
> +    callOnMainThread([this] {

How did you decide a smart pointer (weak pointer or strong pointer) was not needed here?

> Source/WebCore/platform/mock/MediaPlaybackTargetPickerMock.cpp:79
> +    callOnMainThread([this, weakThis = makeWeakPtr(*this)] {
> +        if (!weakThis)
> +            return;

I prefer the style where we turn weakThis into a RefPtr for the duration of the lambda over the more tricky style where we capture this twice, for both weak and strong pointers. And this is test code so it’s not performance-sensitive.

[Chris Dumez]

Comment on attachment 430639 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/TextTrackRepresentationCocoa.mm:155
>> +            weakThis->client().textTrackRepresentationBoundsChanged(weakThis->bounds());
> 
> Is bounds() guaranteed to not do any work that could have the side effect of destroying this?

bounds() is a fairly simple getter, I don't see any way it can destroy |this|.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:182
>> +    callOnMainThread([this] {
> 
> How did you decide a smart pointer (weak pointer or strong pointer) was not needed here?

This class is  a singleton.

>> Source/WebCore/platform/mock/MediaPlaybackTargetPickerMock.cpp:79
>> +            return;
> 
> I prefer the style where we turn weakThis into a RefPtr for the duration of the lambda over the more tricky style where we capture this twice, for both weak and strong pointers. And this is test code so it’s not performance-sensitive.

Sounds a lot like the ObjC pattern. Something like this?
RefPtr protectedThis { weakThis.get() };
if (!protectedThis)
    return;

// ...

[Chris Dumez]

Comment on attachment 430639 [details]
Patch

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

>>> Source/WebCore/platform/mock/MediaPlaybackTargetPickerMock.cpp:79
>>> +            return;
>> 
>> I prefer the style where we turn weakThis into a RefPtr for the duration of the lambda over the more tricky style where we capture this twice, for both weak and strong pointers. And this is test code so it’s not performance-sensitive.
> 
> Sounds a lot like the ObjC pattern. Something like this?
> RefPtr protectedThis { weakThis.get() };
> if (!protectedThis)
>     return;
> 
> // ...

If I don't capture |this|, it means adding quite a few protectedThis-> here and it other lambda in this file. The previous code was not protecting |this| at all either.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 430639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430639&action=review
> 
> >>> Source/WebCore/platform/mock/MediaPlaybackTargetPickerMock.cpp:79
> >>> +            return;
> >> 
> >> I prefer the style where we turn weakThis into a RefPtr for the duration of the lambda over the more tricky style where we capture this twice, for both weak and strong pointers. And this is test code so it’s not performance-sensitive.
> > 
> > Sounds a lot like the ObjC pattern. Something like this?
> > RefPtr protectedThis { weakThis.get() };
> > if (!protectedThis)
> >     return;
> > 
> > // ...
> 
> If I don't capture |this|, it means adding quite a few protectedThis-> here
> and it other lambda in this file. The previous code was not protecting
> |this| at all either.

I am not a fan of the result:
```
void MediaPlaybackTargetPickerMock::startingMonitoringPlaybackTargets()
{
    LOG(Media, "MediaPlaybackTargetPickerMock::startingMonitoringPlaybackTargets");

    callOnMainThread([weakThis = makeWeakPtr(*this)] {
        RefPtr protectedThis { weakThis.get() };
        if (!protectedThis)
            return;

        if (protectedThis->m_state == MediaPlaybackTargetContext::MockState::OutputDeviceAvailable)
            protectedThis->availableDevicesDidChange();

        if (!protectedThis->m_deviceName.isEmpty() && protectedThis->m_state != MediaPlaybackTargetContext::MockState::Unknown)
            protectedThis->currentDeviceDidChange();
    });
}
```

Also, I have just found out this class is not even RefCounted :)

[Darin Adler]

Comment on attachment 430639 [details]
Patch

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

>>>> Source/WebCore/platform/mock/MediaPlaybackTargetPickerMock.cpp:79
>>>> +            return;
>>> 
>>> I prefer the style where we turn weakThis into a RefPtr for the duration of the lambda over the more tricky style where we capture this twice, for both weak and strong pointers. And this is test code so it’s not performance-sensitive.
>> 
>> Sounds a lot like the ObjC pattern. Something like this?
>> RefPtr protectedThis { weakThis.get() };
>> if (!protectedThis)
>>     return;
>> 
>> // ...
> 
> If I don't capture |this|, it means adding quite a few protectedThis-> here and it other lambda in this file. The previous code was not protecting |this| at all either.

I understand if you don’t want to go for it; when I said "review+" I didn’t mean you have to make changes based on my comments (just consider them). I’d write my new code that way, but it’s probably asking you too much to do it to this existing code.

To avoid all the protectedThis-> we’d need a member function to call from the lambda, which might defeat the points of lambda in coding clarity. But to me object lifetime clarity is one of the most important kinds!

[EWS]

Committed r278522 (238521@main): <https://commits.webkit.org/238521@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430639 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78899303>