[MediaSession] Add playbackState attribute to Blink MediaSession and use it to determine playback s…

[MediaSession] Add playbackState attribute to Blink MediaSession and use it to determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470
Committed: https://crrev.com/8fae3446b59330bb28aba760c7dbde3d7e80a6f7
Cr-Commit-Position: refs/heads/master@{#439375}

[Zhiqiang Zhang (Slow), 2016-12-15 20:36:01]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-15 20:36:02]

PTAL :)

[mlamouri (slow - plz ping), 2016-12-16 15:49:14]

lgtm with comments applied.

Also, can you add LayoutTests to test the API surface:
- setting/getting the playbackState works
- setting the playback state to some random value doesn't work
- the default value is 'none'

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:179: // Called when the
advertised playback state of a MediaSessionService has
s/advertised//

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:181: void
OnMediaSessionAdvertisedPlaybackStateChanged(
ditto

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.h (right):

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.h:33: }
It's generally better to avoid inline implementations in the header but I guess
this is fine because the rest of the getters do the same.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:83:
NOTREACHED();
No need for `default`, it will not compile if we add a new value which will
force us to handle it.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:85: return
WTF::emptyAtom;
NOTREACHED()?

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:5: //
https://wicg.github.io/mediasession/#the-mediasession-interface
Can you put this above the actual interface and a link for the enum above the
enum?

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom
(right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:24:
// Spec: https://wicg.github.io/mediasession/
I don't think this is needed.

[Zhiqiang Zhang (Slow), 2016-12-16 18:26:15]

On 2016/12/16 15:49:14, mlamouri wrote:
> lgtm with comments applied.
> 
> Also, can you add LayoutTests to test the API surface:
> - setting/getting the playbackState works
> - setting the playback state to some random value doesn't work
> - the default value is 'none'

Done

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:179: // Called when the
advertised playback state of a MediaSessionService has
On 2016/12/16 15:49:13, mlamouri wrote:
> s/advertised//

Done.

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:181: void
OnMediaSessionAdvertisedPlaybackStateChanged(
On 2016/12/16 15:49:14, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.h (right):

https://codereview.chromium.org/2583463002/diff/30001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.h:33: }
On 2016/12/16 15:49:14, mlamouri wrote:
> It's generally better to avoid inline implementations in the header but I
guess
> this is fine because the rest of the getters do the same.

Acknowledged.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:83:
NOTREACHED();
On 2016/12/16 15:49:14, mlamouri wrote:
> No need for `default`, it will not compile if we add a new value which will
> force us to handle it.

Done.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:85: return
WTF::emptyAtom;
On 2016/12/16 15:49:14, mlamouri wrote:
> NOTREACHED()?

Done.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:5: //
https://wicg.github.io/mediasession/#the-mediasession-interface
On 2016/12/16 15:49:14, mlamouri wrote:
> Can you put this above the actual interface and a link for the enum above the
> enum?

Done.

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom
(right):

https://codereview.chromium.org/2583463002/diff/30001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:24:
// Spec: https://wicg.github.io/mediasession/
On 2016/12/16 15:49:14, mlamouri wrote:
> I don't think this is needed.

Done.

[Zhiqiang Zhang (Slow), 2016-12-16 18:30:36]

Description was changed from

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and propagates
the attribute through mojo to MediaSessionImpl. The attribute is then
used to determine the actual playback state.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470
==========

to

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470
==========

[Zhiqiang Zhang (Slow), 2016-12-16 18:36:12]

zqzhang@chromium.org changed reviewers:
+ meacer@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-16 18:36:12]

+meacer@: mojo changes

[Zhiqiang Zhang (Slow), 2016-12-16 19:02:39]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 19:03:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 19:55:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:55:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[meacer, 2016-12-16 21:08:01]

mojom lgtm

[Zhiqiang Zhang (Slow), 2016-12-16 21:44:14]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-16 21:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2583463002/#ps110001
(title: "fixed tests")

[commit-bot: I haz the power, 2016-12-16 21:45:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 23:48:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 23:48:05]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-12-17 19:44:15]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-17 19:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 20:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-17 20:17:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-12-17 22:36:13]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-17 22:36:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 00:24:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-18 00:24:38]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-12-18 14:37:09]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-18 14:37:10]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2583463002/#ps170001
(title: "trying to fix tests")

[commit-bot: I haz the power, 2016-12-18 14:37:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 14:41:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-18 14:41:37]

Failed to apply patch for
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:79
  error: third_party/WebKit/Source/modules/mediasession/MediaSession.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
Index: third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
diff --git a/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
b/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
index
b723323b95aebfa0441d76589fd913e89b82099c..78b896924de72a0e229a1c3ff971b037b1678f47
100644
--- a/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
+++ b/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
@@ -29,8 +29,6 @@ const AtomicString& mojomActionToEventName(MediaSessionAction
action) {
       return EventTypeNames::play;
     case MediaSessionAction::PAUSE:
       return EventTypeNames::pause;
-    case MediaSessionAction::PLAY_PAUSE:
-      return EventTypeNames::playpause;
     case MediaSessionAction::PREVIOUS_TRACK:
       return EventTypeNames::previoustrack;
     case MediaSessionAction::NEXT_TRACK:
@@ -51,8 +49,6 @@ WTF::Optional<MediaSessionAction> eventNameToMojomAction(
     return MediaSessionAction::PLAY;
   if (EventTypeNames::pause == eventName)
     return MediaSessionAction::PAUSE;
-  if (EventTypeNames::playpause == eventName)
-    return MediaSessionAction::PLAY_PAUSE;
   if (EventTypeNames::previoustrack == eventName)
     return MediaSessionAction::PREVIOUS_TRACK;
   if (EventTypeNames::nexttrack == eventName)
@@ -66,10 +62,40 @@ WTF::Optional<MediaSessionAction> eventNameToMojomAction(
   return WTF::nullopt;
 }
 
+const AtomicString& mediaSessionPlaybackStateToString(
+    mojom::blink::MediaSessionPlaybackState state) {
+  DEFINE_STATIC_LOCAL(const AtomicString, noneValue, ("none"));
+  DEFINE_STATIC_LOCAL(const AtomicString, pausedValue, ("paused"));
+  DEFINE_STATIC_LOCAL(const AtomicString, playingValue, ("playing"));
+
+  switch (state) {
+    case mojom::blink::MediaSessionPlaybackState::NONE:
+      return noneValue;
+    case mojom::blink::MediaSessionPlaybackState::PAUSED:
+      return pausedValue;
+    case mojom::blink::MediaSessionPlaybackState::PLAYING:
+      return playingValue;
+  }
+  NOTREACHED();
+  return WTF::emptyAtom;
+}
+
+mojom::blink::MediaSessionPlaybackState stringToMediaSessionPlaybackState(
+    const String& stateName) {
+  if (stateName == "none")
+    return mojom::blink::MediaSessionPlaybackState::NONE;
+  if (stateName == "paused")
+    return mojom::blink::MediaSessionPlaybackState::PAUSED;
+  DCHECK_EQ(stateName, "playing");
+  return mojom::blink::MediaSessionPlaybackState::PLAYING;
+}
+
 }  // anonymous namespace
 
 MediaSession::MediaSession(ExecutionContext* executionContext)
-    : ContextLifecycleObserver(executionContext), m_clientBinding(this) {}
+    : ContextLifecycleObserver(executionContext),
+      m_playbackState(mojom::blink::MediaSessionPlaybackState::NONE),
+      m_clientBinding(this) {}
 
 MediaSession* MediaSession::create(ExecutionContext* executionContext) {
   return new MediaSession(executionContext);
@@ -79,6 +105,17 @@ void MediaSession::dispose() {
   m_clientBinding.Close();
 }
 
+void MediaSession::setPlaybackState(const String& playbackState) {
+  m_playbackState = stringToMediaSessionPlaybackState(playbackState);
+  mojom::blink::MediaSessionService* service = getService();
+  if (service)
+    service->SetPlaybackState(m_playbackState);
+}
+
+String MediaSession::playbackState() {
+  return mediaSessionPlaybackStateToString(m_playbackState);
+}
+
 void MediaSession::setMetadata(MediaMetadata* metadata) {
   if (mojom::blink::MediaSessionService* service = getService()) {
     service->SetMetadata(MediaMetadataSanitizer::sanitizeAndConvertToMojo(

[Zhiqiang Zhang (Slow), 2016-12-18 16:18:01]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-12-18 16:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2583463002/#ps190001
(title: "rebased")

[commit-bot: I haz the power, 2016-12-18 16:18:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 18:41:01]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1482077881263010,
"parent_rev": "3c94c74a74ec07d8a119e759816d5178f5d62701", "commit_rev":
"5423f25981860dda246f56c8cdbc0187d0056d5e"}

[commit-bot: I haz the power, 2016-12-18 18:41:27]

Description was changed from

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470
==========

to

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470

Review-Url: https://codereview.chromium.org/2583463002
==========

[commit-bot: I haz the power, 2016-12-18 18:41:29]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-12-18 18:43:55]

Description was changed from

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470

Review-Url: https://codereview.chromium.org/2583463002
==========

to

==========
[MediaSession] Add playbackState attribute to Blink MediaSession and use it to
determine playback state

This CL implements MediaSession.playbackState attribute, and
propagates the attribute through mojo to MediaSessionImpl. The
attribute is then used to determine the actual playback state.

Basically, the behavior change is: when the page media is
currently paused while playbackState is "playing", we show
a "pause" button instead of "playing" button.

Spec PR:
https://github.com/WICG/mediasession/pull/152

BUG=674470

Committed: https://crrev.com/8fae3446b59330bb28aba760c7dbde3d7e80a6f7
Cr-Commit-Position: refs/heads/master@{#439375}
==========

[commit-bot: I haz the power, 2016-12-18 18:43:56]

Patchset 11 (id:??) landed as
https://crrev.com/8fae3446b59330bb28aba760c7dbde3d7e80a6f7
Cr-Commit-Position: refs/heads/master@{#439375}