[webkit-reviews] review granted: [Bug 172897] [MediaStream] Page capture state not reported correctly : [Attachment 311935] 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 172897: [MediaStream] Page capture state not reported correctly
https://bugs.webkit.org/show_bug.cgi?id=172897

Attachment 311935: Proposed patch.

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




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

Looks like an improvement.
Would we have an issue if the capturing track is no longer part of the media
stream using removeTrack?
Maybe that calls for a more direct access to the capturing sources of the
document or maybe better of the WebProcess.

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:348
> +	   if (track->kind() == "audio") {

Probably more efficient to use track->source().type()

> LayoutTests/fast/mediastream/media-stream-track-muted.html:5
> +    <title>Mute video and audio tracks independently, make sure page state
updates correctly.</title>

Cool test!

> LayoutTests/fast/mediastream/media-stream-track-muted.html:41
> +	      
assert_false(internals.pageMediaState().includes('HasMutedAudioCaptureDevice'))
;

Might want to reject if window.internals is undefined.

> LayoutTests/fast/mediastream/media-stream-track-muted.html:77
> +	       testTrack(stream.getAudioTracks()[0], resolve, reject);

You could probably create the promise_test inside testTrack function and then
have:
testTrack(stream.getVideoTracks()[0],  "Mute video track only")
testTrack(stream.getAudioTracks()[0],  "Mute audio track only")