[webkit-reviews] review granted: [Bug 172168] [MediaStream] Return default device list until user gives permission to capture : [Attachment 310263] Proposed 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 172168: [MediaStream] Return default device list until user gives
permission to capture
https://bugs.webkit.org/show_bug.cgi?id=172168

Attachment 310263: Proposed patch.

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 310263
  --> https://bugs.webkit.org/attachment.cgi?id=310263
Proposed patch.

LGTM.
Some nits below.

View in context: https://bugs.webkit.org/attachment.cgi?id=310263&action=review

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:79
> +void MediaDevicesRequest::filterDeviceList(Vector<RefPtr<MediaDeviceInfo>>&
devices)

Should be a Vector<Ref<MediaDeviceInfo>>&

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:91
> +    static const int defaultMicrophoneCount = 1;

unsigned?

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:95
> +    devices.removeAllMatching([&](RefPtr<MediaDeviceInfo> device) -> bool {

Can we change "RefPtr<MediaDeviceInfo> device" in "const
RefPtr<MediaDeviceInfo>& device"?
Or better "const Ref<MediaDeviceInfo>&?
Would be good to have it "const MediaDeviceInfo&" but I don't think it will
work.

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:136
> +	       filterDeviceList(devices);

devices can be made a Vector<Ref<MediaDeviceInfo>>.
Small optim: use reserveInitialCapacity/uncheckedAppend.
Except that if id is empty, we do not append a device info, why is it the case?

I see in above code the same (originHasPersistentAccess ||
document.hasHadActiveMediaStreamTrack()) if statement for handling the label.
Would it make sense to remove the label in filterDeviceList as well?

> LayoutTests/fast/mediastream/media-devices-enumerate-devices.html:22
> +		   assert_not_equals(device.kind, null, "device.groupId is
non-null");

If you would like to have more output logging, you could put all these asserts
in a test(() => { }) and give it a dedicated name.

> LayoutTests/fast/mediastream/media-devices-enumerate-devices.html:53
> +		   testRunner.setUserMediaPersistentPermissionForOrigin(false,
document.location.href, "");

Out of curiosity, what happens if this test is run several times on the same
web process. Will this option persist for the first promise test?

> LayoutTests/fast/mediastream/media-devices-enumerate-devices.html:58
> +			   .then((devices) => {

To remove increasing indentation, you can do 
return navigator.mediaDevices.getUserMedia({audio:{}, video:{}})
.then((stream) => {
  return navigator.mediaDevices.enumerateDevices();
}.then((devices) => {
   assert_equals(...)
}