Bug 180206 – [MediaStream] Use CaptureDevice instead of device ID to identify devices

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Use CaptureDevice instead of device ID to identify devices when requesting user permission.

[Radar WebKit Bug Importer]

<rdar://problem/35775758>

[Eric Carlson]

Created attachment 328001 [details]
Proposed patch.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Eric Carlson]

Created attachment 328018 [details]
Updated patch.

[Eric Carlson]

Created attachment 328025 [details]
Updated patch.

[youenn fablet]

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<>?

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 328098 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 328098 [details]
Patch for landing.

Clearing flags on attachment: 328098

Committed r225395: <https://trac.webkit.org/changeset/225395>