[webkit-reviews] review granted: [Bug 208651] Implement setWirelessPlaybackTarget, performTaskAtMediaTime, and wouldTaintOrigin in GPUProcess : [Attachment 392609] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 208651: Implement setWirelessPlaybackTarget, performTaskAtMediaTime, and
wouldTaintOrigin in GPUProcess
https://bugs.webkit.org/show_bug.cgi?id=208651

Attachment 392609: Patch

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




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

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

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:81
> +    m_performTaskAtMediaTimeCompletionHandler = nullptr;

We should call this completion handler otherwise we will get some debug assert.
Maybe the completion handler should return an Optional<Time>, WTF::null opt
meaning that it was not done.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:797
> +void RemoteMediaPlayerProxy::performTaskAtMediaTime(MediaTime&& taskTime,
WallTime&& messageTime, CompletionHandler<void(const MediaTime&)>&&
completionHandler)

I don't think we need a MediaTime&& since it only contains int values. Ditto
for WallTime that we can simply pass by value.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:808
> +    m_player->performTaskAtMediaTime([this, weakThis = makeWeakPtr(this)] {

It might be just simpler to have the lambda capture completionHandler so that
we do not have to store it in m_performTaskAtMediaTimeCompletionHandler, which
could be overridden by two successive
RemoteMediaPlayerProxy::performTaskAtMediaTime calls.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:817
> +void RemoteMediaPlayerProxy::wouldTaintOrigin(struct
WebCore::SecurityOriginData originData,
CompletionHandler<void(Optional<bool>)>&& completionHandler)

s/struct WebCore::SecurityOriginData/const SecurityOriginData&/

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:848
> +    if
(!connection().sendSync(Messages::RemoteMediaPlayerProxy::WouldTaintOrigin(orig
in.data()),
Messages::RemoteMediaPlayerProxy::WouldTaintOrigin::Reply(wouldTaint), m_id))

Could there be a way to use m_cachedState to remove the sync IPC as a
follow-up?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1034
> +    m_performTaskAtMediaTimeCompletionHandler = WTFMove(completionHandler);

There is the risk that performTaskAtMediaTime is called twice so that we would
override a non null m_performTaskAtMediaTimeCompletionHandler.
It might be simpler to just capture completionHandler in the lambda passed to
sendWithAsyncReply.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:353
> +    bool performTaskAtMediaTime(WTF::Function<void()>&&, const MediaTime&)
final;

s/WTF:://

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:373
> +    WTF::Function<void()> m_performTaskAtMediaTimeCompletionHandler;

s/WTF:://