[webkit-reviews] review granted: [Bug 192268] [MediaStream] 'devicechange' event when more capture device information are revealed. : [Attachment 356324] 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 192268: [MediaStream] 'devicechange' event when more capture device
information are revealed.
https://bugs.webkit.org/show_bug.cgi?id=192268

Attachment 356324: Patch

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




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

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

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:108
> +    auto completion = [this, protectedThis = makeRef(*this),
canAccessMicrophone, canAccessCamera] (const Vector<CaptureDevice>&
captureDevices, const String& deviceIdentifierHashSalt, bool) mutable {

I guess a follow-up patch will remove the second parameter of the completion
handler.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:189
> +    m_hasFilteredDeviceList = false;

Should it be m_hasFilteredDeviceList = true?
Maybe we should add a test, something like a page which calls getUserMedia,
reload, and does enumerateDevice.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
> +	       continue;

I wonder whether we should update this check.
It seems that if we have something like:
1. a page A with a same origin frame B
2. frame B does getUserMedia and user agrees.
3. frame B is navigated to same origin and does again getUserMedia (or page A
makes a getUserMedia request).
4. second getUserMedia will create a reprompt.

Maybe frameID check should be applied to the main frame only?
Probably not to be changed in this patch.
Or maybe we should always pass the mainFrameId and no longer the frameId?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:428
> +    static const int defaultCameraCount = 2;

Is IOS_FAMILY too large for 2 cameras?
I am thinking watch/iosmac.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:452
> +	   for (auto& device : devices) {

const auto&?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:459
> +		   if (device.type() ==
WebCore::CaptureDevice::DeviceType::Microphone && ++microphoneCount >
defaultMicrophoneCount)

Ah so maybe defaultMicrophoneCount should be renamed to
defaultMaxMicrophoneCount.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:472
> +	       filteredDevices.append(CaptureDevice(id, device.type(), label,
groupId));

We could make CaptureDevice constructor take String&&

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:477
> +	  
m_page.process().send(Messages::WebPage::DidCompleteMediaDeviceEnumeration(user
MediaID, WTFMove(filteredDevices), WTFMove(deviceIDHashSalt),
WTFMove(originHasPersistentAccess)), m_page.pageID());

We do not need to move originHasPersistentAccess.
We could try removing it.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:546
> +    m_hasFilteredDeviceList = false;

Should it be m_hasFilteredDeviceList = true?

> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:25
> +		   resolve();

Should we call reject("navigator.mediaDevices.ondevicechange took too long")
instead of console.log/resolve?
I guess resolve() allows to continue running the test but
assert_equals(eventCount, 1) will always fail so we do not gain much.

> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:30
> +	   assert_equals(eventCount, 1, "one event fired");

Maybe this eventCount should be checked at the end of the test, after the
second enumerateDevices call.