Implement setWirelessPlaybackTarget, performTaskAtMediaTime, and wouldTaintOrigin
<rdar://problem/60088298>
Created attachment 392592 [details] Patch
Created attachment 392593 [details] Patch
Created attachment 392609 [details] Patch
Comment on attachment 392609 [details] 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(origin.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:://
Comment on attachment 392609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392609&action=review Thanks for the 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. Great suggestion, I have made this change. >> 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. Fixed. >> 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. MediaPlayerPrivateAVFoundationObjC is the only media player to implement this callback so far, and it only keeps one pending callback at a time. I have added a comment explaining this. >> 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&/ Oops, copy pasta FTW! Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:848 >> + if (!connection().sendSync(Messages::RemoteMediaPlayerProxy::WouldTaintOrigin(origin.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? As we discussed offline, this is possible for some canvas elements so I'll do that in the patch for landing. I have also added a { SecurityOriginData, Optional<bool> } cache to MediaPlayerPrivateRemote, so we will only need to make the sync call once for a given origin in any case. >> 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. Fixed >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:353 >> + bool performTaskAtMediaTime(WTF::Function<void()>&&, const MediaTime&) final; > > s/WTF::// Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:373 >> + WTF::Function<void()> m_performTaskAtMediaTimeCompletionHandler; > > s/WTF::// Removed.
Created attachment 392894 [details] Patch for landing
Comment on attachment 392894 [details] Patch for landing Clearing flags on attachment: 392894 Committed r258082: <https://trac.webkit.org/changeset/258082>