[webkit-reviews] review granted: [Bug 126780] Allow MediaSessionManager to restrict media playback : [Attachment 220902] Updated patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted  review:
Bug 126780: Allow MediaSessionManager to restrict media playback
https://bugs.webkit.org/show_bug.cgi?id=126780

Attachment 220902: Updated patch
https://bugs.webkit.org/attachment.cgi?id=220902&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220896&action=review


I don't really like the name "ElementMediaSession".  If we move WebAudio out of
modules, most of it will end up in html/, not platform/, and conceivably we
could want to add a html/-level MediaSession for something which is not an
Element.

Aside from that, this looks good with a couple of nits:

> Source/WebCore/html/ElementMediaSession.cpp:125
> +void ElementMediaSession::clientWillBeginPlayback()
> +{
> +    MediaSessionManager::sharedManager().sessionWillBeginPlayback(*this);
> +}

This doesn't appear to have any HTMLMediaElement specific knowledge.  Can we
move this into MediaSession?

> Source/WebCore/html/ElementMediaSession.cpp:130
> +void ElementMediaSession::pauseSession()
> +{
> +    client().pausePlayback();
> +}

This looks like it does exactly the same thing as MediaSession::pauseSession().
 Can we get rid of this and make MediaSession::pauseSession() non-virtual?

> Source/WebCore/platform/audio/MediaSessionManager.cpp:142
> +    for (auto* oneSession : m_sessions) {
> +	   if (oneSession == &session)
> +	       continue;
> +	   if (oneSession->mediaType() != sessionType)
> +	       continue;
> +	   if (restrictions & ConcurrentPlaybackNotPermitted)
> +	       oneSession->pauseSession();
> +    }

It's a tiny optimization, but to benefit those platforms which will never set
the ConcurrentPlaybackNotPermitted restriction, the first check should be: "if
(!(restrictions & ConcurrentPlaybackNotPermitted)) continue;"