[webkit-reviews] review granted: [Bug 180666] Playing WebRTC video tracks should prevent from display to going to sleep : [Attachment 329021] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 180666: Playing WebRTC video tracks should prevent from display to going to
sleep
https://bugs.webkit.org/show_bug.cgi?id=180666

Attachment 329021: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 329021
  --> https://bugs.webkit.org/attachment.cgi?id=329021
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6634
> +    // In case of remote media stream video tracks, audio might be rendered
outside the corresponding video element.

This comment is mysterious. The code below says that if there is a non-capture,
non-canvas video track it should prevent sleep. Is the reason it prevents sleep
is that it *might* be playing audio? That really doesn’t explain things well to
me.

The comment here should explain why the code below implements the correct rule,
and I don’t think the comment does that. Generally speaking it seems we have
risky behavior here, where we prevent sleep just because something *might* be
audible or visible. That’s not so great, and hard to understand.

> Source/WebCore/html/HTMLMediaElement.cpp:6638
> +    bool hasRemoteMediaStreamVideoTrack = m_mediaStreamSrcObject &&
WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
> +	   return track->privateTrack().type() ==
RealtimeMediaSource::Type::Video && !track->isCaptureTrack() &&
!track->isCanvas();
> +    });
> +    shouldBeAbleToSleep = shouldBeAbleToSleep &&
!hasRemoteMediaStreamVideoTrack;

Seems wasteful to do this computation if shouldBeAbleToSleep is already false.
Making this a lambda could fix that.

    shouldBeAbleToSleep = shouldBeAbleToSleep && !([m_mediaStreamSrcObject] {
	return m_mediaStreamSrcObject &&
WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
	    return track->privateTrack().type() ==
RealtimeMediaSource::Type::Video && !track->isCaptureTrack() &&
!track->isCanvas();
	});
    }());