Bug 129903 – Add remote control command support to MediaSession.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

Add remote control command support to MediaSession.

[Jer Noble]

Created attachment 226157 [details]
Patch

[Eric Carlson]

Comment on attachment 226157 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5858
> +void HTMLMediaElement::didReceiveRemoteControlCommand(MediaSession::RemoteControlCommandType command)
> +{
> +    UNUSED_PARAM(command);
> +}

A FIXME would be helpful here.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:43
> +    : m_mostRecentlyActive(nullptr)

How about m_currentSession?  I think "current session" makes more sense than "most recently active session"

> Source/WebCore/platform/audio/MediaSessionManager.cpp:106
> +    if (!m_clients.isEmpty() && (session.mediaType() == MediaSession::Video || session.mediaType() == MediaSession::Audio)) {

Nit: this could be an early return.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:124
> +    if (!m_clients.isEmpty() && (session.mediaType() == MediaSession::Video || session.mediaType() == MediaSession::Audio)) {

Ditto.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:166
> +    if (!m_clients.isEmpty() && (session.mediaType() == MediaSession::Video || session.mediaType() == MediaSession::Audio)) {

Ditto.

[Jer Noble]

Created attachment 226602 [details]
Patch

[Eric Carlson]

Comment on attachment 226602 [details]
Patch

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

> Source/WebCore/platform/audio/MediaSessionManager.cpp:172
> +    for (auto& client : m_clients)
> +        client->didBeginPlayback();

Shouldn't this be above the "if (!restrictions & ConcurrentPlaybackNotPermitted)" above?

> Source/WebCore/platform/audio/MediaSessionManager.cpp:211
> +    if (!m_currentSession)
> +        return;

if (!m_currentSession || !m_currentSession->canReceiveRemoteControlCommands())

[Jer Noble]

Created attachment 226620 [details]
Patch

[Eric Carlson]

Comment on attachment 226620 [details]
Patch

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

> Source/WebCore/platform/audio/MediaSessionManager.cpp:171
> +    m_activeSession = &session;

This is unnecessary, we already called setCurrentSession() above.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:177
> +    for (auto& client : m_clients)
> +        client->didBeginPlayback();

Move up?

> Source/WebCore/platform/audio/MediaSessionManager.cpp:217
> +    if (!m_activeSession)
> +        return;
> +    m_activeSession->didReceiveRemoteControlCommand(command);

canReceiveRemoteControlCommands()?

[Jer Noble]

Created attachment 226622 [details]
Patch

[Jer Noble]

Created attachment 226624 [details]
Patch

[Jer Noble]

Committed r165570: <http://trac.webkit.org/changeset/165570>