[webkit-reviews] review granted: [Bug 193230] Define page media state flags for display capture. : [Attachment 358577] 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 193230: Define page media state flags for display capture.
https://bugs.webkit.org/show_bug.cgi?id=193230

Attachment 358577: Patch

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




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

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:164
> +    virtual CaptureDevice::DeviceType deviceType() const { return
CaptureDevice::DeviceType::Unknown; }

Make it virtual pure might be better, that would ensure that every subclass
implements it.

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:54
> +    CaptureDevice::DeviceType deviceType() const final { return
CaptureDevice::DeviceType::Window; }

DeviceType also has Application and Browser in addition to Window.
I wonder whether we should not remove these 3 types and use Screen for all of
them.

One reason is that 'ScreenCaptureMask' refers to screen + the three other
types.
If we need to surface that information in some way, we could surface it outside
of deviceType.

Or maybe ScreenCaptureMask, HasInterruptedScreenCaptureDevice et al should be
renamed to DisplayCaptureMask.

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:60
>	   : RealtimeMediaSource(type, WTFMove(name), WTFMove(sourceID),
WTFMove(hashSalt))

We could compute type based on deviceType.

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:63
> +	   , m_deviceType(deviceType)

Should we assert that deviceType != Unknown.

> LayoutTests/fast/mediastream/get-display-media-muted.html:40
> +	   return new Promise((resolve, reject) => {

await would work too

> LayoutTests/fast/mediastream/get-display-media-muted.html:43
> +		   new Promise((innerResolve, innerReject) => {

This could be rewritten to something like:

track.onmute = () => assert_not_reached(...)
await new Promise((resolve) => { track.onmute = resolve; });
await waitForPageStateChange(10, internals.pageMediaState();
// with waitForPageStateChange as an async function

> LayoutTests/fast/mediastream/get-display-media-muted.html:66
> +

Double line.