[webkit-reviews] review granted: [Bug 180206] [MediaStream] Use CaptureDevice instead of device ID to identify devices : [Attachment 328025] Updated patch.

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 180206: [MediaStream] Use CaptureDevice instead of device ID to identify
devices
https://bugs.webkit.org/show_bug.cgi?id=180206

Attachment 328025: Updated patch.

https://bugs.webkit.org/attachment.cgi?id=328025&action=review




--- Comment #7 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 328025
  --> https://bugs.webkit.org/attachment.cgi?id=328025
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<>?