MediaPlayerPrivateRemote::didLoadingProgress should not send synchronous message to GPU process
rdar://78834312
Created attachment 430821 [details] Patch
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); ?
(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); > ?
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.
(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.
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.
(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.
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.
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&
Created attachment 430932 [details] Patch
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].