[MediaSource] Push capabilities across process boundary during UIProcess capture.
Created attachment 306998 [details] Patch
Created attachment 306999 [details] Patch
Created attachment 307002 [details] Patch
Comment on attachment 307002 [details] Patch Looks fine to me, but a real WK2 reviewer will have to give it an official r+.
Created attachment 307011 [details] Patch for landing
Created attachment 307012 [details] Patch for landing
rdar://problem/31435730
Comment on attachment 307012 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=307012&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:133 > + RealtimeMediaSourceCapabilities() > + { > + } = default; > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:152 > +void UserMediaCaptureManagerProxy::capabilities(uint64_t id, std::optional<WebCore::RealtimeMediaSourceCapabilities>& capabilities) Why not just return a std::optional? > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:161 > + else > + capabilities = std::nullopt; Is it necessary to set capabilities to nullopt here? If so, why isn't it done in the early return? > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:144 > + mutable bool m_capabilitiesInitialized { false }; > + mutable std::optional<RealtimeMediaSourceCapabilities> m_capabilities; This is very strange. std::optional keeps track of whether it is initialized. If you need more states than that, use WTF::Expected. > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250 > + m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0); Why do we need to add another sync message?
(In reply to Alex Christensen from comment #8) > Comment on attachment 307012 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307012&action=review > > > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:133 > > + RealtimeMediaSourceCapabilities() > > + { > > + } > > = default; Ok. > > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:152 > > +void UserMediaCaptureManagerProxy::capabilities(uint64_t id, std::optional<WebCore::RealtimeMediaSourceCapabilities>& capabilities) > > Why not just return a std::optional? Because that makes the message binding code super unhappy. > > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:161 > > + else > > + capabilities = std::nullopt; > > Is it necessary to set capabilities to nullopt here? If so, why isn't it > done in the early return? Yes. > > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:144 > > + mutable bool m_capabilitiesInitialized { false }; > > + mutable std::optional<RealtimeMediaSourceCapabilities> m_capabilities; > > This is very strange. std::optional keeps track of whether it is > initialized. If you need more states than that, use WTF::Expected. Alex and I discussed offline and I may have managed to convince him that std::optional<std::unique_ptr<Capabilities>> is OK. > > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250 > > + m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0); > > Why do we need to add another sync message? Because the caller expects to receive a reply synchronously.
Created attachment 307028 [details] Patch for landing
Created attachment 307064 [details] Patch for landing After discussing this with other members of the WebKit team, it became apparent that the best option was to remove the possibility of returning nullptr from RealtimeMediaSource::capabilities().
Created attachment 307113 [details] Patch for landing One last version to make the GTK bts happy.
Comment on attachment 307113 [details] Patch for landing Clearing flags on attachment: 307113 Committed r215362: <http://trac.webkit.org/changeset/215362>
(In reply to Jer Noble from comment #9) > (In reply to Alex Christensen from comment #8) > > Comment on attachment 307012 [details] > > Patch for landing > > > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250 > > > + m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0); > > > > Why do we need to add another sync message? > > Because the caller expects to receive a reply synchronously. Can you go into more detail about this? Adding more synchronous calls is something that should be taken very seriously, and all alternatives should be discussed before going forward with it. Why can't capabilities be pushed to the WebProcess proactively when the source is created?
Comment on attachment 307002 [details] Patch Unflagging and obsoleting, since there is a newer patch for landing.