Bug 172168 – [MediaStream] Return default device list until user gives permission to capture

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Return a default device list until user gives permission to capture

[Eric Carlson]

<rdar://problem/31816884>

[Eric Carlson]

Created attachment 310263 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 310263 [details]
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(...)
}

[Eric Carlson]

(In reply to youenn fablet from comment #3)
> Comment on attachment 310263 [details]
> 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>>&
> 
OK

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

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

That doesn't work with the promise.

> Small optim: use reserveInitialCapacity/uncheckedAppend.
> Except that if id is empty, we do not append a device info, why is it the
> case?
> 
A device must have an ID.

> 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?
> 
I did it that way at first, but the #ifs make it really difficult to read.

> > 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?
> 
State is reset in TestController::resetStateToConsistentValues.

> > 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(...)
> }

[Eric Carlson]

Created attachment 310273 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 310273 [details]
Patch for landing.

Clearing flags on attachment: 310273

Committed r216938: <http://trac.webkit.org/changeset/216938>

[Ahmad Saleem]

Landed but didn't back out.

https://github.com/WebKit/WebKit/commit/9480fc6f22e6cda01b1f8526daf264d96b31754f

Marking this as "RESOLVED FIXED". Thanks!