[MediaSession] Send play/pause action to the page if the handler is set

[MediaSession] Send play/pause action to the page if the handler is set

Previously we handle play/pause actions internally and do not send them
to the page. We should let the page handle them if it specifies handlers
for play/pause.

BUG=680058
Review-Url: https://codereview.chromium.org/2621333002
Cr-Commit-Position: refs/heads/master@{#444337}
Committed: https://chromium.googlesource.com/chromium/src/+/e6e31527e9ace8760b0b89f738098d8dbf6d12cb

[Zhiqiang Zhang (Slow), 2017-01-11 14:57:36]

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

[Zhiqiang Zhang (Slow), 2017-01-11 14:57:36]

PTAL

Should we also send PAUSE for stop()? I have no strong opinions.

[mlamouri (slow - plz ping), 2017-01-12 12:33:24]

I guess STOP should behave the same way for consistency. It's nothing more than
PAUSE+dismiss.

[Zhiqiang Zhang (Slow), 2017-01-12 17:14:10]

PTAL

[mlamouri (slow - plz ping), 2017-01-16 12:25:50]

https://codereview.chromium.org/2621333002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2621333002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:124:
mMediaSessionObserver.getMediaSession().stop();
I think the case of stop should be different. `onStop()` will be called in many
cases:
- when swiping the notification out, it will call stop but in this case, there
will be no active players.
- When cancelling (only when swiping isn't available), it will call stop but in
this case, we should also close the notification after pausing.
- The proper "stop" action is only for remote so we should probably not care
much.

In the case of swiping and "stop" we shouldn't call the media session pause
action handler because there should be no player or media session. The cancel
case is a bit trickier and I don't think we can handle it properly because we
shouldn't remove the notification if the playback didn't pause.

Because of all of this, maybe the best thing to do would be to not handle stop.
WDYT?

[Zhiqiang Zhang (Slow), 2017-01-16 15:23:06]

PTAL per offline discussion

https://codereview.chromium.org/2621333002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2621333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:123:
&& !mNotificationInfoBuilder.build().isPaused) {
I wish we could merge MediaNotificationInfo and its builder. A clone() method
should be enough to avoid accidental changes.

[Zhiqiang Zhang (Slow), 2017-01-16 17:21:01]

https://codereview.chromium.org/2621333002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2621333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:125:
.didReceiveAction(MediaSessionAction.PAUSE);
I just realize maybe we need to revert the changes to stop(). The reason is that
otherwise, there will be an inconsistency between the MediaSessionImpl state and
the notification state, i.e. MediaSessionImpl is SUSPENDED but the notification
is hidden.

This would cause the following issues:
- MediaSessionAndroid is still active after the notification is dismissed, so
that it can still receive media keys events.
- Audio focus is not abandoned when dismissing the notification.

WDYT?

[Zhiqiang Zhang (Slow), 2017-01-16 17:36:48]

On 2017/01/16 17:21:01, Zhiqiang Zhang wrote:
> This would cause the following issues:
> - MediaSessionAndroid is still active after the notification is dismissed, so
> that it can still receive media keys events.

Sorry I was wrong, the notification calls stopSelf() on STOP, so the service and
MediaSessionAndroid will deactivate.

> - Audio focus is not abandoned when dismissing the notification.

This might be a minor problem.

[Zhiqiang Zhang (Slow), 2017-01-17 18:14:47]

PTAL. Reverted changes to STOP

[mlamouri (slow - plz ping), 2017-01-17 18:39:52]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-01-17 18:39:52]

lgtm

[commit-bot: I haz the power, 2017-01-17 18:40:21]

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, 2017-01-17 20:23:15]

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

[commit-bot: I haz the power, 2017-01-17 20:23:16]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Zhiqiang Zhang (Slow), 2017-01-18 11:27:51]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-01-18 11:27:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2621333002/#ps80001
(title: "added nullcheck")

[commit-bot: I haz the power, 2017-01-18 11:28:03]

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, 2017-01-18 12:00:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484738871626320,
"parent_rev": "e2e736937aa73e417e29e9d553444e4aa5bfe929", "commit_rev":
"e6e31527e9ace8760b0b89f738098d8dbf6d12cb"}

[commit-bot: I haz the power, 2017-01-18 12:00:44]

Description was changed from

==========
[MediaSession] Send play/pause action to the page if the handler is set

Previously we handle play/pause actions internally and do not send them
to the page. We should let the page handle them if it specifies handlers
for play/pause.

BUG=680058
==========

to

==========
[MediaSession] Send play/pause action to the page if the handler is set

Previously we handle play/pause actions internally and do not send them
to the page. We should let the page handle them if it specifies handlers
for play/pause.

BUG=680058

Review-Url: https://codereview.chromium.org/2621333002
Cr-Commit-Position: refs/heads/master@{#444337}
Committed:
https://chromium.googlesource.com/chromium/src/+/e6e31527e9ace8760b0b89f73809...
==========

[commit-bot: I haz the power, 2017-01-18 12:00:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e6e31527e9ace8760b0b89f73809...