Use CaptureDevice instead of device ID to identify devices when requesting user permission.
<rdar://problem/35775758>
Created attachment 328001 [details] Proposed patch.
Comment on attachment 328001 [details] Proposed patch. Attachment 328001 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5421399 New failing tests: fast/mediastream/getUserMedia-grant-persistency.html fast/mediastream/getUserMedia-grant-persistency2.html http/tests/media/media-stream/get-user-media-prompt.html
Created attachment 328008 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328018 [details] Updated patch.
Created attachment 328025 [details] Updated patch.
Comment on attachment 328025 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=328025&action=review > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:158 > +void UserMediaRequest::allow(std::optional<CaptureDevice>&& audioDevice, std::optional<CaptureDevice>&& videoDevice, String&& deviceIdentifierHashSalt) An alternative to std::optional would be to consider that there is no CaptureDevice if CaptureDevice.persistentId is the null string or its type is none (i.e. default constructed CaptureDevice). We could also have added an operator bool or helper method to know whether CaptureDevice was valid or not like optional has. Not sure what is best, but optional is fine to me. > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:162 > + m_allowedVideoDevice = videoDevice; We can probably move audioDevice/videoDevice. > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:219 > +void UserMediaPermissionRequestManagerProxy::grantAccess(uint64_t userMediaID, std::optional<CaptureDevice> audioDevice, std::optional<CaptureDevice> videoDevice, const String& deviceIdentifierHashSalt) Should be const std::optional<CaptureDevice>& probably. > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:221 > + UserMediaProcessManager::singleton().willCreateMediaStream(*this, audioDevice.has_value(), videoDevice.has_value()); could use !!audioDevice instead of has_value. > Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.h:54 > + const Vector<String> audioDeviceUIDs() const; Can we use Vector<> instead of const Vector<>?
Comment on attachment 328025 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=328025&action=review >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:158 >> +void UserMediaRequest::allow(std::optional<CaptureDevice>&& audioDevice, std::optional<CaptureDevice>&& videoDevice, String&& deviceIdentifierHashSalt) > > An alternative to std::optional would be to consider that there is no CaptureDevice if CaptureDevice.persistentId is the null string or its type is none (i.e. default constructed CaptureDevice). > We could also have added an operator bool or helper method to know whether CaptureDevice was valid or not like optional has. > Not sure what is best, but optional is fine to me. Good idea. I added an operator bool that checks type != Unknown. >> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:162 >> + m_allowedVideoDevice = videoDevice; > > We can probably move audioDevice/videoDevice. That doesn't work because we have to WTFMove() the device parameters to RealtimeMediaSourceCenter::createMediaStream. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:219 >> +void UserMediaPermissionRequestManagerProxy::grantAccess(uint64_t userMediaID, std::optional<CaptureDevice> audioDevice, std::optional<CaptureDevice> videoDevice, const String& deviceIdentifierHashSalt) > > Should be const std::optional<CaptureDevice>& probably. Fixed. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:221 >> + UserMediaProcessManager::singleton().willCreateMediaStream(*this, audioDevice.has_value(), videoDevice.has_value()); > > could use !!audioDevice instead of has_value. I did that at first but changed to has_value at some point. Reconsidering, I think you are right. >> Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.h:54 >> + const Vector<String> audioDeviceUIDs() const; > > Can we use Vector<> instead of const Vector<>? Fixed.
Created attachment 328098 [details] Patch for landing.
Comment on attachment 328098 [details] Patch for landing. Clearing flags on attachment: 328098 Committed r225395: <https://trac.webkit.org/changeset/225395>