Bug 129903

Summary:

Add remote control command support to MediaSession.

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

Media

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, philipj, sergio

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	eric.carlson: review+

Description Jer Noble 2014-03-07 13:04:30 PST

Add remote control command support to MediaSession.

Comment 1 Jer Noble 2014-03-07 13:26:25 PST

Created attachment 226157 [details]
Patch

Comment 2 Eric Carlson 2014-03-07 15:08:48 PST

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.

Comment 3 Jer Noble 2014-03-13 11:25:31 PDT

Created attachment 226602 [details]
Patch

Comment 4 Eric Carlson 2014-03-13 11:37:50 PDT

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())

Comment 5 Jer Noble 2014-03-13 14:05:15 PDT

Created attachment 226620 [details]
Patch

Comment 6 Eric Carlson 2014-03-13 14:16:28 PDT

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()?

Comment 7 Jer Noble 2014-03-13 14:20:19 PDT

Created attachment 226622 [details]
Patch

Comment 8 Jer Noble 2014-03-13 14:31:34 PDT

Created attachment 226624 [details]
Patch

Comment 9 Jer Noble 2014-03-13 15:04:41 PDT

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