Bug 226761 – MediaPlayerPrivateRemote::didLoadingProgress should not send synchronous message to GPU process

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

MediaPlayerPrivateRemote::didLoadingProgress should not send synchronous message to GPU process

[Jean-Yves Avenard [:jya]]

rdar://78834312

[Jean-Yves Avenard [:jya]]

Created attachment 430821 [details]
Patch

[Chris Dumez]

Comment on attachment 430821 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:428
> +    using DidLoadingProgressCompletionHandler = CompletionHandler<void(bool)>;

Not sure why we need this alias considering it is longer than the underlying type.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:271
> +    m_player->didLoadingProgress([completionHandler = WTFMove(completionHandler)](bool progress) mutable {

Can't this be simplified as:
m_player->didLoadingProgress(WTFMove(completionHandler));
?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:288
> +    ASSERT(false, "Should always be using didLoadingProgressAsync");

ASSERT_NOT_REACHED()

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
> +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {

can this be:
connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), WTFMove(callback), m_id);
?

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 430821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430821&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:428
> > +    using DidLoadingProgressCompletionHandler = CompletionHandler<void(bool)>;
> 
> Not sure why we need this alias considering it is longer than the underlying
> type.
> 
> > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:271
> > +    m_player->didLoadingProgress([completionHandler = WTFMove(completionHandler)](bool progress) mutable {
> 
> Can't this be simplified as:
> m_player->didLoadingProgress(WTFMove(completionHandler));
> ?
> 
> > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:288
> > +    ASSERT(false, "Should always be using didLoadingProgressAsync");
> 
> ASSERT_NOT_REACHED()

or ASSERT_NOT_REACHED_WITH_MESSAGE("Should always be using didLoadingProgressAsync")

> 
> > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
> > +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {
> 
> can this be:
> connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::
> DidLoadingProgress(), WTFMove(callback), m_id);
> ?

[Jean-Yves Avenard [:jya]]

Comment on attachment 430821 [details]
Patch

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

>>> Source/WebCore/platform/graphics/MediaPlayer.h:428
>>> +    using DidLoadingProgressCompletionHandler = CompletionHandler<void(bool)>;
>> 
>> Not sure why we need this alias considering it is longer than the underlying type.
> 
> or ASSERT_NOT_REACHED_WITH_MESSAGE("Should always be using didLoadingProgressAsync")

so if we ever change the type in the future, you don't have to edit it everywhere. I think it's good practice.

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Comment on attachment 430821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430821&action=review
> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.h:428
> >>> +    using DidLoadingProgressCompletionHandler = CompletionHandler<void(bool)>;
> >> 
> >> Not sure why we need this alias considering it is longer than the underlying type.
> > 
> > or ASSERT_NOT_REACHED_WITH_MESSAGE("Should always be using didLoadingProgressAsync")
> 
> so if we ever change the type in the future, you don't have to edit it
> everywhere. I think it's good practice.

OK, if that's your preference. I don't feel strongly about having an alias or not.

[Jean-Yves Avenard [:jya]]

Comment on attachment 430821 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
>>> +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {
>> 
>> can this be:
>> connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), WTFMove(callback), m_id);
>> ?
> 
> 

It can.
I guess, conceptually I've seen them as different, it just happens to be the same underlying type.

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 430821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430821&action=review
> 
> >>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
> >>> +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {
> >> 
> >> can this be:
> >> connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), WTFMove(callback), m_id);
> >> ?
> > 
> > 
> 
> It can.
> I guess, conceptually I've seen them as different, it just happens to be the
> same underlying type.

If you create a lambda whose only thing it does is calling a lambda, then it's a tell-tell sign you may not need to wrap the lambda.

[Jean-Yves Avenard [:jya]]

Comment on attachment 430821 [details]
Patch

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

>>>>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
>>>>> +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {
>>>> 
>>>> can this be:
>>>> connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), WTFMove(callback), m_id);
>>>> ?
>>> 
>>> 
>> 
>> It can.
>> I guess, conceptually I've seen them as different, it just happens to be the same underlying type.
> 
> If you create a lambda whose only thing it does is calling a lambda, then it's a tell-tell sign you may not need to wrap the lambda.

There are so many ways you can define a template with varying parameters; yet they are to be used in the same manner, that it's unfortunately far from being the norm. 
Luckily, WK's IPC bindings makes it much easier than the ones I'm used to work with.

[Jean-Yves Avenard [:jya]]

Comment on attachment 430821 [details]
Patch

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

>>>>>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:294
>>>>>> +    connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), [callback = WTFMove(callback)](bool progress) mutable {
>>>>> 
>>>>> can this be:
>>>>> connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::DidLoadingProgress(), WTFMove(callback), m_id);
>>>>> ?
>>>> 
>>>> 
>>> 
>>> It can.
>>> I guess, conceptually I've seen them as different, it just happens to be the same underlying type.
>> 
>> If you create a lambda whose only thing it does is calling a lambda, then it's a tell-tell sign you may not need to wrap the lambda.
> 
> There are so many ways you can define a template with varying parameters; yet they are to be used in the same manner, that it's unfortunately far from being the norm. 
> Luckily, WK's IPC bindings makes it much easier than the ones I'm used to work with.

I meant earlier: conceptually, the IPC's method is called with DidLoadingProgressAsyncReply, the other MediaPlayer::DidLoadingProgressCompletionHandler&

[Jean-Yves Avenard [:jya]]

Created attachment 430932 [details]
Patch

[EWS]

Committed r278646 (238628@main): <https://commits.webkit.org/238628@main>

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