Clean up RealtimeMediaSource interfaces
<rdar://problem/36108648>
Created attachment 329702 [details] Patch
Comment on attachment 329702 [details] Patch Attachment 329702 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5735540 New failing tests: imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html http/tests/ssl/media-stream/get-user-media-nested.html http/tests/media/media-stream/disconnected-frame.html http/tests/ssl/media-stream/get-user-media-different-host.html
Created attachment 329718 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 329702 [details] Patch Attachment 329702 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5735692 New failing tests: imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html http/tests/ssl/media-stream/get-user-media-nested.html http/tests/ssl/media-stream/get-user-media-different-host.html
Created attachment 329719 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 329773 [details] Patch
Comment on attachment 329773 [details] Patch Some minor comments. View in context: https://bugs.webkit.org/attachment.cgi?id=329773&action=review > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:226 > + m_request.videoConstraints.deviceIDHashSalt = WTFMove(deviceIdentifierHashSalt); Probably not for this patch, but since we have a request there, maybe it should contain the deviceIdentifierHashSalt directly. > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:280 > + return; I am not sure we can do that. If we are in contextDestroyed() it means document is in its destructor, so I am not sure we can do things right there. I am about to change it to an ActiveDOMObject and change it to stop(). In that case, the implementation is probably safe. > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:286 > + controller->cancelUserMediaAccessRequest(*this); Why do we need to keep a ref there? Should we instead protect it outside the if? > Source/WebCore/Modules/mediastream/UserMediaRequest.h:94 > + Ref<UserMediaRequest> m_userRequest; Should it be m_userMediaRequest then? > Source/WebCore/platform/mediastream/MediaStreamRequest.h:49 > + return (decoder.decodeEnum(request.type) && decoder.decode(request.audioConstraints) && decoder.decode(request.videoConstraints)); We now try to use the modern IPC decoder. The signature is std::optional<MediaStreamRequest> decode(Decoder& decoder). > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:141 > + result.append(device); We should probably have done this? Were we listing/using devices that are not enabled? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:57 > + for (auto& device : captureDevices()) { Maybe you could use findMatching. > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:50 > CaptureSourceOrError createVideoCaptureSource(const CaptureDevice& device, const MediaConstraints* constraints) final Maybe we could pass the persistentId instead of the CaptureDevice? > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:182 > capabilities.addFacingMode(RealtimeMediaSourceSettings::Environment); Use a ternary? > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:243 > +void UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(uint64_t userMediaID, uint64_t frameID, Ref<SecurityOrigin>&& userMediaDocumentOrigin, Ref<SecurityOrigin>&& topLevelDocumentOrigin, const MediaStreamRequest& userRequest) You can probably use a MediaStreamRequest&& by changing also the signature in WebPageProxy (IPC allows taking an &&). A follow-up might be better since tis patch is pretty big. > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:98 > + SecurityOrigin* topLevelDocumentOrigin = userRequest.topLevelDocumentOrigin(); auto* ? > Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:99 > ASSERT(topLevelDocumentOrigin); Not sure we need this assert, or we should add one for userMediaDocumentOrigin as well?
Comment on attachment 329773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329773&action=review >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:226 >> + m_request.videoConstraints.deviceIDHashSalt = WTFMove(deviceIdentifierHashSalt); > > Probably not for this patch, but since we have a request there, maybe it should contain the deviceIdentifierHashSalt directly. OK >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:286 >> + controller->cancelUserMediaAccessRequest(*this); > > Why do we need to keep a ref there? Should we instead protect it outside the if? Fixed. >> Source/WebCore/Modules/mediastream/UserMediaRequest.h:94 >> + Ref<UserMediaRequest> m_userRequest; > > Should it be m_userMediaRequest then? Fixed. >> Source/WebCore/platform/mediastream/MediaStreamRequest.h:49 >> + return (decoder.decodeEnum(request.type) && decoder.decode(request.audioConstraints) && decoder.decode(request.videoConstraints)); > > We now try to use the modern IPC decoder. > The signature is > std::optional<MediaStreamRequest> decode(Decoder& decoder). Fixed. >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:141 >> + result.append(device); > > We should probably have done this? Were we listing/using devices that are not enabled? No, but this will make sense in the next patch. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:57 >> + for (auto& device : captureDevices()) { > > Maybe you could use findMatching. I will fix this in a future patch. >> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:50 >> CaptureSourceOrError createVideoCaptureSource(const CaptureDevice& device, const MediaConstraints* constraints) final > > Maybe we could pass the persistentId instead of the CaptureDevice? Good idea, I will do that i the next patch. >> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:182 >> capabilities.addFacingMode(RealtimeMediaSourceSettings::Environment); > > Use a ternary? This will also make sense in the next patch. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:243 >> +void UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(uint64_t userMediaID, uint64_t frameID, Ref<SecurityOrigin>&& userMediaDocumentOrigin, Ref<SecurityOrigin>&& topLevelDocumentOrigin, const MediaStreamRequest& userRequest) > > You can probably use a MediaStreamRequest&& by changing also the signature in WebPageProxy (IPC allows taking an &&). > A follow-up might be better since tis patch is pretty big. Will do. >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:98 >> + SecurityOrigin* topLevelDocumentOrigin = userRequest.topLevelDocumentOrigin(); > > auto* ? Fixed. >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:99 >> ASSERT(topLevelDocumentOrigin); > > Not sure we need this assert, or we should add one for userMediaDocumentOrigin as well? Fixed.
Created attachment 329807 [details] Patch
Comment on attachment 329807 [details] Patch Rejecting attachment 329807 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 329807, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: urce/WebKit/UIProcess/WebPageProxy.cpp Hunk #3 succeeded at 5987 (offset 4 lines). patching file Source/WebKit/UIProcess/WebPageProxy.h patching file Source/WebKit/UIProcess/WebPageProxy.messages.in patching file Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp patching file Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/5758601
Created attachment 329820 [details] Patch
Created attachment 329853 [details] Patch
Comment on attachment 329853 [details] Patch Clearing flags on attachment: 329853 Committed r226160: <https://trac.webkit.org/changeset/226160>
All reviewed patches have been landed. Closing bug.
Plus r226178 to fix builds when MEDIA_STREAM is not defined (bug 181026).
http://trac.webkit.org/r226197
(In reply to Alex Christensen from comment #17) > http://trac.webkit.org/r226197 There's also a Source/WebKit/UIProcess/WebPageProxy.cpp.orig fwiw
Thanks! http://trac.webkit.org/r226334