[webkit-reviews] review granted: [Bug 188521] [MediaStream] Move capture device monitoring to WebKit : [Attachment 347229] 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 188521: [MediaStream] Move capture device monitoring to WebKit
https://bugs.webkit.org/show_bug.cgi?id=188521

Attachment 347229: Patch

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




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

Nice patch!
A couple of nits and questions below.

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:252
> +	   }

This is another patch so should probably be removed ;)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:199
> +    m_deviceChangedObserver = WTFMove(observer);

Maybe we should ASSERT(!m_deviceChangedObserver)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:103
> +    std::function<void()> m_deviceChangedObserver;

Maybe WTF::Function in case we need it in the future?

>
Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:1
45
> +    deviceChanged();

Potential preexisting issue and probably not a big deal, but is there a risk
that we generate device change events more than needed?
Let's say we call refreshAudioCaptureDevices but
m_devices/m_audioSessionCaptureDevices remain the same for instance.

> Source/WebCore/platform/mock/MockRealtimeMediaSource.cpp:100
> +    RealtimeMediaSourceCenter::singleton().captureDevicesChanged();

Why is this change needed? Ditto for other changes.

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:286
> +	       auto oldDevices = m_captureDevices;

WTFMove(m_captureDevices)

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:295
> +		       auto oldDevice = std::find_if(oldDevices.begin(),
oldDevices.end(), [&newDevice] (auto& device) {

can be written as:
if (oldDevices.findMatching([&newDevice] (auto& device) {...} == notFound)
  haveChanges = true;

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:43
> +static WebCore::ActivityState::Flags focusedActiveWindow =
WebCore::ActivityState::IsFocused | WebCore::ActivityState::WindowIsActive |
WebCore::ActivityState::IsVisible;

We are adding IsVisible while the spec and name of the variable sats focused
and active. Is there a reason for that?

>
Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:232
> +    m_deviceChangeObserverMap.set(identifier, WTFMove(observer));

s/set/add/

>
Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:275
> +    }

I guess the idea is to protect for the case of the map being modified while
iterating through it.
s/it/iterator.
Another approach would be to make modifications to the map happening
asynchronously in addDeviceChangeObserver/removeDeviceChangeObserver.

>
Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:289
> +    });

Why does it need to be asynchronously removed? We could also do that lazily and
never remove the observer until it gets destroyed.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:72
> +    void activityStateDidChange(WebCore::ActivityState::Flags
oldActivityState, WebCore::ActivityState::Flags newActivityState) override;

final?

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:91
> +    bool m_monitoringStateChanges { false };

m_monitoringStateChanges without an s? maybe m_monitoringActivityStateChange?

> LayoutTests/fast/mediastream/device-change-event-2.html:74
> +	   await new Promise((resolve, reject) => {

Maybe add assert_true(document.hidden) here.

> LayoutTests/fast/mediastream/device-change-event-2.html:108
> +	   assert_equals(eventCount, 1, "one one event fired");

s/one one/one