Implement MediaSession on top of the WebMediaPlayerDelegate.

Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.

Committed: https://crrev.com/bb3eaacc70007d44dc7788fcbbe106977f49b066
Cr-Commit-Position: refs/heads/master@{#371870}

[DaleCurtis, 2016-01-08 01:56:37]

dalecurtis@chromium.org changed reviewers:
+ mlamouri@chromium.org, timav@chromium.org

[DaleCurtis, 2016-01-08 01:56:37]

Mounir, as discussed this morning -- let me know your high level feedback. I'm
especially keen to hear your thoughts on playback permission given that it
appears to add a lot of complexity for no purpose (in a world without autoplay).

This still needs some more testing (there's a crash on backgrounding Chrome w/
spitzer) but otherwise works.

[DaleCurtis, 2016-01-08 02:07:17]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement. Adding latency for this does not
seem to make sense (regardless of what the AudioManager docs say).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

[DaleCurtis, 2016-01-08 02:19:59]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement. Adding latency for this does not
seem to make sense (regardless of what the AudioManager docs say).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency for this round trip does not seem to make sense
given Chrome's usage model and the restrictions on playback. Even
in an autoplay world, in practice, propagating back the pause event
is fast enough in all cases with WMPA and most cases (aside from
furious tapping) to avoid any playout.

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

[DaleCurtis, 2016-01-08 02:20:52]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency for this round trip does not seem to make sense
given Chrome's usage model and the restrictions on playback. Even
in an autoplay world, in practice, propagating back the pause event
is fast enough in all cases with WMPA and most cases (aside from
furious tapping) to avoid any playout.

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases (aside from furious tapping) to avoid any
audible playout (even using obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

[DaleCurtis, 2016-01-08 02:22:03]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases (aside from furious tapping) to avoid any
audible playout (even using obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases to avoid any audible playout (even using
obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

[DaleCurtis, 2016-01-08 02:31:48]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases to avoid any audible playout (even using
obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases with WMPI to avoid any audible playout (even
using obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

[mlamouri (slow - plz ping), 2016-01-08 15:08:43]

On 2016/01/08 at 01:56:37, dalecurtis wrote:
> Mounir, as discussed this morning -- let me know your high level feedback. I'm
especially keen to hear your thoughts on playback permission given that it
appears to add a lot of complexity for no purpose (in a world without autoplay).

RequestPlay() isn't a permission system. I saw it was named with "permission"
somewhere but it's not what this call does. What's happening is that, on
Android, we get audio focus in order to play sound. The audio focus request can
be rejected by the system, for example if the user is in a call. In which case,
we should not play anything. Playing and then stopping seems like the wrong
approach here even though I understand this is adding a slight delay.

Maybe a solution would be to take this code path only on Android? we could have
a method that says whether we should request playback (which would only return
true on Android) or simply #ifdef the hole thing?

[whywhat, 2016-01-08 15:21:57]

On 2016/01/08 at 15:08:43, mlamouri wrote:
> On 2016/01/08 at 01:56:37, dalecurtis wrote:
> > Mounir, as discussed this morning -- let me know your high level feedback.
I'm especially keen to hear your thoughts on playback permission given that it
appears to add a lot of complexity for no purpose (in a world without autoplay).
> 
> RequestPlay() isn't a permission system. I saw it was named with "permission"
somewhere but it's not what this call does. What's happening is that, on
Android, we get audio focus in order to play sound. The audio focus request can
be rejected by the system, for example if the user is in a call. In which case,
we should not play anything. Playing and then stopping seems like the wrong
approach here even though I understand this is adding a slight delay.
> 
> Maybe a solution would be to take this code path only on Android? we could
have a method that says whether we should request playback (which would only
return true on Android) or simply #ifdef the hole thing?

Isn't the same auto focus system available on Windows 10 and maybe Mac? We don't
use it yet but should at least acknowledge that we might change the behavior in
the future (so #ifdef-ing Android is probably still fine short term).

[mlamouri (slow - plz ping), 2016-01-08 15:29:45]

On 2016/01/08 at 15:21:57, avayvod wrote:
> On 2016/01/08 at 15:08:43, mlamouri wrote:
> > On 2016/01/08 at 01:56:37, dalecurtis wrote:
> > > Mounir, as discussed this morning -- let me know your high level feedback.
I'm especially keen to hear your thoughts on playback permission given that it
appears to add a lot of complexity for no purpose (in a world without autoplay).
> > 
> > RequestPlay() isn't a permission system. I saw it was named with
"permission" somewhere but it's not what this call does. What's happening is
that, on Android, we get audio focus in order to play sound. The audio focus
request can be rejected by the system, for example if the user is in a call. In
which case, we should not play anything. Playing and then stopping seems like
the wrong approach here even though I understand this is adding a slight delay.
> > 
> > Maybe a solution would be to take this code path only on Android? we could
have a method that says whether we should request playback (which would only
return true on Android) or simply #ifdef the hole thing?
> 
> Isn't the same auto focus system available on Windows 10 and maybe Mac? We
don't use it yet but should at least acknowledge that we might change the
behavior in the future (so #ifdef-ing Android is probably still fine short
term).

That was definitely my intent when I suggested a method or an #ifdef. Whatever
solution we come with, extending it to support a new platform should be little
work compared to the actual new platform support ;)

[DaleCurtis, 2016-01-08 18:42:18]

Well focus or permission, it's a rose by any other name :) In both cases we're
asking if we should play audio or not and aborting or proceeding in tune. One
could argue that pausing afterward is more in line with a difference of
definition between the two :p

I realize pausing-after is a bit counter-intuitive, but in terms of user visible
impact there is little difference between the two approaches (audio won't start
in the background, the user would have to be on the page and clicking play to
get even an accidental sound -- at which point the immediate pause/no playback
actually feels incorrect?) and in terms of code complexity its a big win (see
the contortions the MediaCodecPlayer went through and consider what we'd need to
add for spitzer).

If we really need to wait for the focus first, long term, I don't feel that
blocking play within the player is the right solution for stream based renderers
on any platform since it can't work for WebAudio, WebRTC, or NaCl/PPAPI. I think
instead we'll want to integrate into AudioOutputController::Play() which is the
first place prior to audio playback (for all producers modulo the Android ones)
that we have asynchronous control:

https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

That typically hops from the IO to the audio thread, which is an okay place to
add an asynchronous focus request.  Even better would be if we can avoid any UI
thread restriction for requesting focus and instead request directly from the
audio or io thread. Otherwise a CancellableClosure could be used.

[Tima Vaisburd, 2016-01-08 19:50:59]

On 2016/01/08 15:08:43, Mounir Lamouri wrote:
> What's happening is that, on
> Android, we get audio focus in order to play sound. The audio focus request
can
> be rejected by the system, for example if the user is in a call. In which
case,
> we should not play anything.

As far as I understand on Android the playback start is a user-requested action,
at least now. Do you expect a user would initiate a playback during the phone
call?
I can imagine it can happen by mistake somehow, of course. Do you have some
other
scenarios in mind where the playback initiated by a used will be rejected?

[mlamouri (slow - plz ping), 2016-01-11 17:14:37]

On 2016/01/08 at 18:42:18, dalecurtis wrote:
> Well focus or permission, it's a rose by any other name :) In both cases we're
asking if we should play audio or not and aborting or proceeding in tune. One
could argue that pausing afterward is more in line with a difference of
definition between the two :p
> 
> I realize pausing-after is a bit counter-intuitive, but in terms of user
visible impact there is little difference between the two approaches (audio
won't start in the background, the user would have to be on the page and
clicking play to get even an accidental sound -- at which point the immediate
pause/no playback actually feels incorrect?) and in terms of code complexity its
a big win (see the contortions the MediaCodecPlayer went through and consider
what we'd need to add for spitzer).
> 
> If we really need to wait for the focus first, long term, I don't feel that
blocking play within the player is the right solution for stream based renderers
on any platform since it can't work for WebAudio, WebRTC, or NaCl/PPAPI. I think
instead we'll want to integrate into AudioOutputController::Play() which is the
first place prior to audio playback (for all producers modulo the Android ones)
that we have asynchronous control:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...
> 
> That typically hops from the IO to the audio thread, which is an okay place to
add an asynchronous focus request.  Even better would be if we can avoid any UI
thread restriction for requesting focus and instead request directly from the
audio or io thread. Otherwise a CancellableClosure could be used.

I understand. That sounds painful but ultimately, if we play before requesting
audio focus, we are going against Android platform conventions and development
guide. Anton and I have been working on making Chrome behave more like any other
native app with regards to media on Android, it would be sad if we were to
regress that.

This said, I doubt rejecting playback is common (should probably add an UMA to
check). However, my point regarding playing during a call still applies. It is a
misconception to assume that the user has to go to the page and press play in
order to have something to play. Pages can use different events to make audio
files ready to play. This is used by youtube to queue videos and music websites
do the same. A bug (or a feature) might end having a website playing audio while
in the background.

You can test this here: http://mounirlamouri.github.io/sandbox/media-focus.html
- Press "delay playback"
- Go back to home screen
- Audio will start soon

RequestPlay() should definitely stay. It's probably fine to make it async if we
have a real plan to make it back synchronous.

[mlamouri (slow - plz ping), 2016-01-11 17:31:52]

I've created a CL that add an UMA to check how ofter requestAudioFocus() fails.

[mlamouri (slow - plz ping), 2016-01-11 17:32:00]

On 2016/01/11 at 17:31:52, Mounir Lamouri wrote:
> I've created a CL that add an UMA to check how ofter requestAudioFocus()
fails.

https://codereview.chromium.org/1579613003

[DaleCurtis, 2016-01-12 02:09:08]

On 2016/01/11 17:14:37, Mounir Lamouri wrote:
> I understand. That sounds painful but ultimately, if we play before requesting
> audio focus, we are going against Android platform conventions and development
> guide. Anton and I have been working on making Chrome behave more like any
other
> native app with regards to media on Android, it would be sad if we were to
> regress that.

I'm curious if given Chrome's usage model if focus is _ever_ rejected. Have you
seen it happen in practice? It'll be interesting to see what the UMA says. If it
never happens or happens < 1% of the time I think the play->pause approach is
fine.

> 
> This said, I doubt rejecting playback is common (should probably add an UMA to
> check). However, my point regarding playing during a call still applies. It is
a
> misconception to assume that the user has to go to the page and press play in
> order to have something to play. Pages can use different events to make audio
> files ready to play. This is used by youtube to queue videos and music
websites
> do the same. A bug (or a feature) might end having a website playing audio
while
> in the background.

You're right I forgot that we allow playlists to continue playing on desktop and
we'd probably do the same thing on Android with background playback. So it's
definitely not true that no background audio could ever start -- though it's
unlikely since the render process will probably get nuked without any audio
streams keeping it alive.

> 
> You can test this here:
http://mounirlamouri.github.io/sandbox/media-focus.html
> - Press "delay playback"
> - Go back to home screen
> - Audio will start soon
> 
> RequestPlay() should definitely stay. It's probably fine to make it async if
we
> have a real plan to make it back synchronous.

Here's the plan I mentioned which would eventually allow us to support focus on
other platforms and for non-WMP streams:

https://codereview.chromium.org/1580493004/

I haven't hooked back up RequestPlay there, but it gives you an idea of what
we'd likely want to do.

[mlamouri (slow - plz ping), 2016-01-14 15:43:08]

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:25: class
MediaSessionController : public MediaSessionObserver {
Maybe this could be in a separate file?

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:79: if
(!session->IsSuspended())
How do you end up with a suspended session that pauses a player?

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:296: void
MediaWebContentsObserverAndroid::OnMediaDestroyedNotification(
Do we destroy the media as soon as it reached the end?

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:302: void
MediaWebContentsObserverAndroid::OnMediaPlayingNotification(
Are we going to call this when the media _starts_ or when it changes status to
play? Something important is that we don't take audio focus until we are
actually starting playback. I would like to not regress that.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.h:58: //
Handles messages for the media player delgate; does not modify the handled
typo: delegate

[DaleCurtis, 2016-01-14 18:21:18]

Just comments for now. I'll clean up after branch cut.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:25: class
MediaSessionController : public MediaSessionObserver {
On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> Maybe this could be in a separate file?

Yeah, I was thinking the same since it ended up larger than expected.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:79: if
(!session->IsSuspended())
On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> How do you end up with a suspended session that pauses a player?

This is due to the delegate sending a "pause" ack back from the renderer after
the session pauses.  It's a bit convoluted to avoid it on the renderer side
since each impl calls DidPause differently.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:296: void
MediaWebContentsObserverAndroid::OnMediaDestroyedNotification(
On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> Do we destroy the media as soon as it reached the end?

No, this is only called when the WMP is being destructed which only occurs in
teardown or when the htmlmediaelement object is gc'd/takes a clearing action.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:302: void
MediaWebContentsObserverAndroid::OnMediaPlayingNotification(
On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> Are we going to call this when the media _starts_ or when it changes status to
> play? Something important is that we don't take audio focus until we are
> actually starting playback. I would like to not regress that.

This is only called when status changes to play.

[DaleCurtis, 2016-01-14 22:46:20]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2016-01-14 23:11:04]

Patchset #2 (id:40001) has been deleted

[DaleCurtis, 2016-01-14 23:15:54]

Actually had time to cleanup today; still haven't pulled out
MediaSessionController into a new file though, will do that later.

[mlamouri (slow - plz ping), 2016-01-19 16:59:11]

Could you rebase and run the tests? I think it's bitrotted :(

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:79: if
(!session->IsSuspended())
On 2016/01/14 at 18:21:18, DaleCurtis wrote:
> On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> > How do you end up with a suspended session that pauses a player?
> 
> This is due to the delegate sending a "pause" ack back from the renderer after
the session pauses.  It's a bit convoluted to avoid it on the renderer side
since each impl calls DidPause differently.

zqzhang@ has been working on issues related to that. We are still DCHECK-ing for
!IsSuspended but we no longer DCHECK if the player isn't part of the session
(but early return instead).

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:302: void
MediaWebContentsObserverAndroid::OnMediaPlayingNotification(
On 2016/01/14 at 18:21:18, DaleCurtis wrote:
> On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> > Are we going to call this when the media _starts_ or when it changes status
to
> > play? Something important is that we don't take audio focus until we are
> > actually starting playback. I would like to not regress that.
> 
> This is only called when status changes to play.

Does that match the 'playing' event?

https://codereview.chromium.org/1570043002/diff/60001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1570043002/diff/60001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:169: virtual bool
IsPlayingRemotely(int player_id);
FWIW, zqzhang@ has a similar change.

https://codereview.chromium.org/1570043002/diff/60001/content/common/media/me...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/1570043002/diff/60001/content/common/media/me...
content/common/media/media_player_messages_android.h:71:
IPC_STRUCT_MEMBER(int64_t, delegate_id)
If I understand correctly `delegate_id` is there only for the WMPA code path?

https://codereview.chromium.org/1570043002/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.h (right):

https://codereview.chromium.org/1570043002/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.h:8: #include <set>
nit: remains from old code?

[DaleCurtis, 2016-01-20 00:43:58]

Didn't have time to do any further cleanup today, just rebased.

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/1/content/browser/media/andro...
content/browser/media/android/media_web_contents_observer_android.cc:302: void
MediaWebContentsObserverAndroid::OnMediaPlayingNotification(
On 2016/01/19 16:59:11, Mounir Lamouri wrote:
> On 2016/01/14 at 18:21:18, DaleCurtis wrote:
> > On 2016/01/14 15:43:08, Mounir Lamouri wrote:
> > > Are we going to call this when the media _starts_ or when it changes
status
> to
> > > play? Something important is that we don't take audio focus until we are
> > > actually starting playback. I would like to not regress that.
> > 
> > This is only called when status changes to play.
> 
> Does that match the 'playing' event?

Yes

https://codereview.chromium.org/1570043002/diff/60001/content/common/media/me...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/1570043002/diff/60001/content/common/media/me...
content/common/media/media_player_messages_android.h:71:
IPC_STRUCT_MEMBER(int64_t, delegate_id)
On 2016/01/19 16:59:11, Mounir Lamouri wrote:
> If I understand correctly `delegate_id` is there only for the WMPA code path?

Correct.

https://codereview.chromium.org/1570043002/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_webmediaplayer_delegate.h (right):

https://codereview.chromium.org/1570043002/diff/60001/content/renderer/media/...
content/renderer/media/renderer_webmediaplayer_delegate.h:8: #include <set>
On 2016/01/19 16:59:11, Mounir Lamouri wrote:
> nit: remains from old code?

Done.

[DaleCurtis, 2016-01-21 04:33:27]

Mounir, can you give this another look today? Thanks!

[mlamouri (slow - plz ping), 2016-01-21 17:26:01]

I've been testing the change and there seems to be a lot of small bugs. For
example, sometimes, the notification stays when a page playing a video is
backgrounded then it's possible to play the video by pressing play.

Sometimes, the notification leaves but in that case, resuming the video when
going back to the page do not show the notification again. It seems that you
didn't move the RemovePlayer() call that was in ::SuspendAndReleaseResources()
or did I miss it?

It also requires some rebase because the cast change has been reverted and
zqzhang@ landed a new method in MediaSessionObserver.

[DaleCurtis, 2016-01-21 18:26:25]

Thanks for the testing. I think I know the root cause there. I'll clean this up
and add tests / ensure it passes the existing mediasession tests by EOD.

[DaleCurtis, 2016-01-22 02:15:54]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Kills the notion of requesting permission for playback before
playback starts in favor of pausing playback afterwards if
permission is not granted (since that is the most common case,
especially so in a world without autoplay like Android). We have
consistent data from YT, that like search, playback delays reduce
watch time and user engagement.

Adding latency (and a ton of complexity) for this round trip does
not seem to make sense given Chrome's usage model and the
restrictions on playback. Even in an autoplay world, in practice,
propagating back the pause event is fast enough in all cases with
WMPA and most cases with WMPI to avoid any audible playout (even
using obnoxious clips at full volume).

BUG=529887
TEST=media sessions work in both wmpi and wmpa.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-22 02:15:54]

dalecurtis@chromium.org changed reviewers:
+ nasko@chromium.org, sandersd@chromium.org

[DaleCurtis, 2016-01-22 02:18:34]

PTAL. All MediaSession tests now pass (including some new ones) and I've
manually verified the background behavior.

nasko: Please review non-media content/ changes and the messages changes.
sandersd: Please review WebMediaPlayerChanges.
timav: Please review MediaPlayerManager changes.
mlamouri: Please review MediaSession changes.

Thanks all!

[DaleCurtis, 2016-01-22 02:32:41]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-01-22 02:32:42]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2016-01-22 02:32:56]

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

[commit-bot: I haz the power, 2016-01-22 02:33:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570043002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570043002/100001

[commit-bot: I haz the power, 2016-01-22 02:35:58]

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

[commit-bot: I haz the power, 2016-01-22 02:35:59]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2016-01-22 04:39:15]

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

[commit-bot: I haz the power, 2016-01-22 04:40:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570043002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570043002/120001

[DaleCurtis, 2016-01-22 04:50:24]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[commit-bot: I haz the power, 2016-01-22 08:53:49]

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

[commit-bot: I haz the power, 2016-01-22 08:53:50]

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

[mlamouri (slow - plz ping), 2016-01-22 16:58:31]

It sgtm apart from from the fact that you don't try to add to the media session
a player that used to be there in order to re-activate it.

Also, there are still issues with video in the backround:
- wmpi: the playback resumes automatically when the page is visible again (the
current behaviour is for the user to press play).
- wmpa: when a video is backgrounded, the session stays active.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:170: virtual bool
IsPlayingRemotely(int player_id);
You are going to hate us but you will have to rebase. This has changed :)

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:23: // These objects
are only created on the UI thread, so this is safe.
`DCHECK_CURRENTLY_ON(BrowserThread::UI);` or it's a lie :)

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:50:
MediaSessionController::~MediaSessionController() {
style: keep implementations in the same order as the declarations.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:51: if (initialized_)
{
nit:
```
if (!initialized_)
  return;
// do stuff
```

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller.h (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:10: #include
"content/browser/media/media_web_contents_observer.h"
nit: I think you should be able to fwd clare this.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:33: void
PausePlayback();
nit: why not `OnPausePlayback()`. It sounds like this method will actually pause
the playback.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:42: bool has_audio() {
return has_audio_; }
nit: `const`

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:44: int
get_player_id_for_testing() { return player_id_; }
nit: `const`

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:48:
MediaWebContentsObserver* const media_web_contents_observer_;
Can you explain the ownership of this in a comment. I guess here just say that
`media_web_contents_observer_` owns this object.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller_unittest.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:68: bool
VerifyMessagePlayPause() {
nit: really a naming issue here. "Verify" sounds like the EXPECT_* is run in
there but it's actually not. Maybe you should rename
"ReceivedMessagePlayPaus()"?

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:83: bool
VerifyMessageVolumeMultiplierUpdate(double expected_multiplier) {
ditto

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:131:
ASSERT_TRUE(VerifyMessagePlayPause<FrameMsg_MediaDelegatePause>());
nit: why ASSERT_TRUE?

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:135:
ASSERT_TRUE(VerifyMessagePlayPause<FrameMsg_MediaDelegatePlay>());
ditto

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.cc:263: //
instance for this playback attempt.
I think this is going to lead to bugs. If you play a media then pause it, the
session stays active indeed. However, it can be suspended from the UI in which
case I think you will not reactivate it. I believe this can be tested easily.
Calling ::Initialize() for a player that is already part of the media session
should be a no-op and will work around this problem.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:45: // the
BrowserMediaPlayerManagers.
You reference multiple time this TODO. Maybe you should file a bug and add it to
the TODO comments so it can be tracked?

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1793:
setVolume(volume_);
I would consider having an UpdateVolume() method. But fine with having that in a
follow-up given that it would probably increase the size of the CL even more.

[nasko, 2016-01-22 17:28:33]

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.cc:139: //
TODO(dalecurtis): These should no longer be FrameHostMsg.
Well, you are adding some new messages. Why make them FrameHostMsg when you can
make them the right type you have in mind?

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:62: //
Handles messages for the media player delegate; does not modify the handled
nit: Is "media player delegate" a type? If yes, why not use the type name
directly?

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:100: // Map
of renderer side media players to session controllers.
nit: s/renderer side/renderer process/

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:245:
static_cast<RenderFrameImpl*>(RenderFrame::FromRoutingID(frame_id))
Why not call RenderFrameImpl::FromRoutingID and avoid the static cast?

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/renderer_webmediaplayer_delegate.cc:85: Observer**
observer) {
Wouldn't returning a pointer to the observer be simpler? Looking at the code,
false means there is no observer and true means the output is set.

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/renderer_webmediaplayer_delegate.cc:88: Observer* result
= reinterpret_cast<Observer*>(player_cookie);
Using pointer values for cookies should not be done. It leaks heap layout
information across processes and is not guaranteed uniqueness, especially with
PartitionAlloc. Generating a random number and having a map from it to observer
is better approach.

[DaleCurtis, 2016-01-22 17:59:44]

Hmm, Mounir can you talk a bit more about the device you're testing on and how
you're backgrounding the video? I verified yesterday it was doing the correct
thing on my N5 when tapping the home button or app switcher.

The android bot is unhappy though, despite all those tests passing locally last
night, so maybe the latest rebase broke something.

[DaleCurtis, 2016-01-22 18:00:37]

Also the resume on return for WMPI is an intentional decision (pre-existing this
change), we can address that in a separate patch set.

[DaleCurtis, 2016-01-22 23:47:51]

Patchset #6 (id:140001) has been deleted

[DaleCurtis, 2016-01-22 23:56:26]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-22 23:56:44]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaDelegate where it
can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-23 00:09:55]

Patchset #6 (id:160001) has been deleted

[DaleCurtis, 2016-01-23 00:10:33]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-23 00:18:22]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-23 02:01:28]

Patchset #6 (id:180001) has been deleted

[DaleCurtis, 2016-01-23 02:11:00]

All comments addressed. Tests should be fixed. Was able to reproduce the issue
with the lingering mediasession with WMPA, that's been fixed as well.  PTAL.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:170: virtual bool
IsPlayingRemotely(int player_id);
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> You are going to hate us but you will have to rebase. This has changed :)

Done, but you guys are killing me! :) You might consider holding off on any
other changes until this lands since the approach is changing so much we'll both
end up with more work otherwise.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:23: // These objects
are only created on the UI thread, so this is safe.
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> `DCHECK_CURRENTLY_ON(BrowserThread::UI);` or it's a lie :)

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:50:
MediaSessionController::~MediaSessionController() {
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> style: keep implementations in the same order as the declarations.

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:51: if (initialized_)
{
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit:
> ```
> if (!initialized_)
>   return;
> // do stuff
> ```

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller.h (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:10: #include
"content/browser/media/media_web_contents_observer.h"
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: I think you should be able to fwd clare this.

Done, but requires including WebContentsObserver now for the MediaPlayerId.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:33: void
PausePlayback();
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: why not `OnPausePlayback()`. It sounds like this method will actually
pause
> the playback.

Went with OnPlaybackPaused() since this is reflecting an event that already
happened in the renderer.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:42: bool has_audio() {
return has_audio_; }
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: `const`

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:44: int
get_player_id_for_testing() { return player_id_; }
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: `const`

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller.h:48:
MediaWebContentsObserver* const media_web_contents_observer_;
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> Can you explain the ownership of this in a comment. I guess here just say that
> `media_web_contents_observer_` owns this object.

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_controller_unittest.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:68: bool
VerifyMessagePlayPause() {
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: really a naming issue here. "Verify" sounds like the EXPECT_* is run in
> there but it's actually not. Maybe you should rename
> "ReceivedMessagePlayPaus()"?

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:83: bool
VerifyMessageVolumeMultiplierUpdate(double expected_multiplier) {
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:131:
ASSERT_TRUE(VerifyMessagePlayPause<FrameMsg_MediaDelegatePause>());
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> nit: why ASSERT_TRUE?

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_controller_unittest.cc:135:
ASSERT_TRUE(VerifyMessagePlayPause<FrameMsg_MediaDelegatePlay>());
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.cc
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.cc:139: //
TODO(dalecurtis): These should no longer be FrameHostMsg.
On 2016/01/22 17:28:32, nasko wrote:
> Well, you are adding some new messages. Why make them FrameHostMsg when you
can
> make them the right type you have in mind?

Figured it would grow the CL too much, but it's not that bad. Done. Took the
opportunity to clean them up a bit too naming and parameter wise.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.cc:263: //
instance for this playback attempt.
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> I think this is going to lead to bugs. If you play a media then pause it, the
> session stays active indeed. However, it can be suspended from the UI in which
> case I think you will not reactivate it. I believe this can be tested easily.
> Calling ::Initialize() for a player that is already part of the media session
> should be a no-op and will work around this problem.

I think this actually makes things a bit more complicated, but since the
complexity is contained to the MediaSessionController (where it's nicely
testable) it's preferable.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:45: // the
BrowserMediaPlayerManagers.
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> You reference multiple time this TODO. Maybe you should file a bug and add it
to
> the TODO comments so it can be tracked?

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:62: //
Handles messages for the media player delegate; does not modify the handled
On 2016/01/22 17:28:32, nasko wrote:
> nit: Is "media player delegate" a type? If yes, why not use the type name
> directly?

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:100: // Map
of renderer side media players to session controllers.
On 2016/01/22 17:28:32, nasko wrote:
> nit: s/renderer side/renderer process/

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:245:
static_cast<RenderFrameImpl*>(RenderFrame::FromRoutingID(frame_id))
On 2016/01/22 17:28:32, nasko wrote:
> Why not call RenderFrameImpl::FromRoutingID and avoid the static cast?

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/android/webmediaplayer_android.cc:1793:
setVolume(volume_);
On 2016/01/22 16:58:31, Mounir Lamouri wrote:
> I would consider having an UpdateVolume() method. But fine with having that in
a
> follow-up given that it would probably increase the size of the CL even more.

Follow up :) I'm trying to avoid breaking a 1000 lines here!

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
File content/renderer/media/renderer_webmediaplayer_delegate.cc (right):

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/renderer_webmediaplayer_delegate.cc:85: Observer**
observer) {
On 2016/01/22 17:28:32, nasko wrote:
> Wouldn't returning a pointer to the observer be simpler? Looking at the code,
> false means there is no observer and true means the output is set.

Done.

https://codereview.chromium.org/1570043002/diff/120001/content/renderer/media...
content/renderer/media/renderer_webmediaplayer_delegate.cc:88: Observer* result
= reinterpret_cast<Observer*>(player_cookie);
On 2016/01/22 17:28:32, nasko wrote:
> Using pointer values for cookies should not be done. It leaks heap layout
> information across processes and is not guaranteed uniqueness, especially with
> PartitionAlloc. Generating a random number and having a map from it to
observer
> is better approach.

Done.

[sandersd (OOO until July 31), 2016-01-25 19:11:00]

lgtm for webmediaplayer_impl.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
File media/blink/webmediaplayer_delegate.h (right):

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_delegate.h:44: // call RemoveObserver() to
unsubscribe from callbacks.
Nit: Should document that this can be called multiple times in a row, since WMPI
relies on that.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:392: int delegate_id_;
Nit: Document |delegate_id_|.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:442: double volume_multiplier_;
Nit: Document |volume_multiplier_|, it's not obvious from context.

[sandersd (OOO until July 31), 2016-01-25 19:12:46]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelgate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-25 21:49:47]

Thanks for review. Comments addressed, single failing test should be resolved as
well.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
File media/blink/webmediaplayer_delegate.h (right):

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_delegate.h:44: // call RemoveObserver() to
unsubscribe from callbacks.
On 2016/01/25 19:10:59, sandersd wrote:
> Nit: Should document that this can be called multiple times in a row, since
WMPI
> relies on that.

Done.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:392: int delegate_id_;
On 2016/01/25 19:10:59, sandersd wrote:
> Nit: Document |delegate_id_|.

Done.

https://codereview.chromium.org/1570043002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:442: double volume_multiplier_;
On 2016/01/25 19:10:59, sandersd wrote:
> Nit: Document |volume_multiplier_|, it's not obvious from context.

Done.

[DaleCurtis, 2016-01-26 02:38:09]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-26 05:26:34]

Patchset #7 (id:220001) has been deleted

[DaleCurtis, 2016-01-26 05:26:44]

Patchset #7 (id:240001) has been deleted

[Tima Vaisburd, 2016-01-26 07:10:14]

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:555:
AddPlayer(player);
Do we need consistency between |players_| and |player_id_to_delegate_id_map_| ?
In this case should AddPlayer() be private?

[DaleCurtis, 2016-01-26 16:17:58]

Hmm, last test is being a pain to fix -- locally the test runs fine, but on the
bots WMPA is completing playback of the 2 second clip in 200ms, so the test
fails while waiting for ended but not paused. Log snippet can be seen here:

https://paste.googleplex.com/6477153843544064?raw

Mounir, Tima: have you seen anything like this before? I haven't verified yet
that the logcat timestamps match the log(error) timestamps, so its possible the
test is actually running for 2 seconds (though that would be surprising).

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:555:
AddPlayer(player);
On 2016/01/26 07:10:14, Tima Vaisburd wrote:
> Do we need consistency between |players_| and |player_id_to_delegate_id_map_|
?
> In this case should AddPlayer() be private?

Yes, that seems reasonable. I don't see anyone using it externally?

[DaleCurtis, 2016-01-26 16:25:18]

adb log timestamps are accurate, chromium timestamps show exactly:

2295444368 - 2295291954 = 152ms have elapsed.

[Tima Vaisburd, 2016-01-26 18:44:20]

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:555:
AddPlayer(player);
On 2016/01/26 16:17:58, DaleCurtis wrote:
> On 2016/01/26 07:10:14, Tima Vaisburd wrote:
> > Do we need consistency between |players_| and
|player_id_to_delegate_id_map_|
> ?
> > In this case should AddPlayer() be private?
> 
> Yes, that seems reasonable. I don't see anyone using it externally?

Neither do I.

[mlamouri (slow - plz ping), 2016-01-26 21:02:50]

lgtm with the session_type bug fixed.

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:41: // the
BrowserMediaPlayerManagers.  Tracked by http://crbug.com/580626
Maybe this comment needs some more explanations. Slow brains like mine might
have a hard time to follow. Now that I understand, maybe something like:
"WebMediaPlayerAndroid isn't aware of audio presence but MediaPlayerManager
knows and will provide the information when calling RequestPlay() which
ultimately will call ::Initialize(). Though, WebMediaPlayer will keep calling
::Initialize() with has_audio set to false so we assume has_audio is true if it
was once true which is the case when has_session_ is true."

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:69: // We already have
a session of the correct type, so do nothing more.
I think this is might lead to bugs. `session_type_` might be the same as
`media_session_type`. Actually, it should most likely be the same unless the
media's length changed. However, that doesn't mean that the `media_session_`
current type is `session_type_`.

BrowserMediaPlayerManager isn't trying to do much logic before calling
`AddPlayer`. I think you should probably do the same here and call `AddPlayer()`
as often as possible and live it to it to decide if the type changed. The thing
changed a bit since it was designed. We used to assume that a player would be
added once but I think calling `AddPlayer()` for an already added player should
have no side effect.

Maybe you should add a unit test for this? :)

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.h (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.h:15: class
MediaWebContentsObserver;
nit: leave an empty line between `namespace` and `class`

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
I think the test fails because of this. We used to pass the test if play() was
successful. Did you try without this? or maybe with another file?

[DaleCurtis, 2016-01-26 21:23:33]

(no changes, just comments)

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:69: // We already have
a session of the correct type, so do nothing more.
On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> I think this is might lead to bugs. `session_type_` might be the same as
> `media_session_type`. Actually, it should most likely be the same unless the
> media's length changed. However, that doesn't mean that the `media_session_`
> current type is `session_type_`.
> 
> BrowserMediaPlayerManager isn't trying to do much logic before calling
> `AddPlayer`. I think you should probably do the same here and call
`AddPlayer()`
> as often as possible and live it to it to decide if the type changed. The
thing
> changed a bit since it was designed. We used to assume that a player would be
> added once but I think calling `AddPlayer()` for an already added player
should
> have no side effect.
> 
> Maybe you should add a unit test for this? :)

I think we want to be careful about having duplicate players in the session;
otherwise we can get duplicate pause/play events which can frustrate users if
they are delivered between a user interaction to the contrary.

I don't follow what's wrong with this code block. If we got to this point
there's already a MediaSession of the right type active (has_session_ must be
true), no? So this should be a no-op other than the fact that it doesn't insert
duplicate player_ids into the MediaSession. Can you elaborate more on what's
wrong here? Also the MediaSessionController::Reinitialize() test should be
covering this -- I'll extend it to include anything we discuss here :)

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> I think the test fails because of this. We used to pass the test if play() was
> successful. Did you try without this? or maybe with another file?

Haven't tried w/o this. It's possible this is changing the timing, but I think
it only let the existing test scrape by with success due to faster timing. The
test is failing because current time jumps from ~16ms to 2.1seconds instantly
for some reason. I'm sending runs to the bots with extra logging to track this
down.

[DaleCurtis, 2016-01-26 21:35:08]

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:69: // We already have
a session of the correct type, so do nothing more.
On 2016/01/26 21:23:33, DaleCurtis wrote:
> On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > I think this is might lead to bugs. `session_type_` might be the same as
> > `media_session_type`. Actually, it should most likely be the same unless the
> > media's length changed. However, that doesn't mean that the `media_session_`
> > current type is `session_type_`.
> > 
> > BrowserMediaPlayerManager isn't trying to do much logic before calling
> > `AddPlayer`. I think you should probably do the same here and call
> `AddPlayer()`
> > as often as possible and live it to it to decide if the type changed. The
> thing
> > changed a bit since it was designed. We used to assume that a player would
be
> > added once but I think calling `AddPlayer()` for an already added player
> should
> > have no side effect.
> > 
> > Maybe you should add a unit test for this? :)
> 
> I think we want to be careful about having duplicate players in the session;
> otherwise we can get duplicate pause/play events which can frustrate users if
> they are delivered between a user interaction to the contrary.
> 
> I don't follow what's wrong with this code block. If we got to this point
> there's already a MediaSession of the right type active (has_session_ must be
> true), no? So this should be a no-op other than the fact that it doesn't
insert
> duplicate player_ids into the MediaSession. Can you elaborate more on what's
> wrong here? Also the MediaSessionController::Reinitialize() test should be
> covering this -- I'll extend it to include anything we discuss here :)

Sorted out offline. MediaSession is a stack type data structure. So even if the
session is of the same type we need to move it back up the stack once playback
starts.

[DaleCurtis, 2016-01-26 21:59:27]

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
On 2016/01/26 21:23:33, DaleCurtis wrote:
> On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > I think the test fails because of this. We used to pass the test if play()
was
> > successful. Did you try without this? or maybe with another file?
> 
> Haven't tried w/o this. It's possible this is changing the timing, but I think
> it only let the existing test scrape by with success due to faster timing. The
> test is failing because current time jumps from ~16ms to 2.1seconds instantly
> for some reason. I'm sending runs to the bots with extra logging to track this
> down.

Looks like MediaPlayerBridge is returning the duration as current time
milliseconds after playback starts....
ERROR:media_player_bridge.cc(519)] OnTimeUpdateTimerFired current_time: 2001000

I think 4.4.4 images may have problems playing these files... Rebuilding my
emulator with the same image the bots use.

[Tima Vaisburd, 2016-01-26 22:12:20]

On 2016/01/26 21:59:27, DaleCurtis wrote:
>
https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
> File
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
> (right):
> 
>
https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
> && DOMUtils.getCurrentTime(webContents, id) > 0;
> On 2016/01/26 21:23:33, DaleCurtis wrote:
> > On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > > I think the test fails because of this. We used to pass the test if play()
> was
> > > successful. Did you try without this? or maybe with another file?
> > 
> > Haven't tried w/o this. It's possible this is changing the timing, but I
think
> > it only let the existing test scrape by with success due to faster timing.
The
> > test is failing because current time jumps from ~16ms to 2.1seconds
instantly
> > for some reason. I'm sending runs to the bots with extra logging to track
this
> > down.
> 
> Looks like MediaPlayerBridge is returning the duration as current time
> milliseconds after playback starts....
> ERROR:media_player_bridge.cc(519)] OnTimeUpdateTimerFired current_time:
2001000
> 
> I think 4.4.4 images may have problems playing these files... Rebuilding my
> emulator with the same image the bots use.

The MediaPlayerBridge should just call getCurrentPosition
http://developer.android.com/reference/android/media/MediaPlayer.html#getCurr...
periodically. Somehow the current position is at the end...

[DaleCurtis, 2016-01-27 00:17:08]

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
On 2016/01/26 21:59:27, DaleCurtis wrote:
> On 2016/01/26 21:23:33, DaleCurtis wrote:
> > On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > > I think the test fails because of this. We used to pass the test if play()
> was
> > > successful. Did you try without this? or maybe with another file?
> > 
> > Haven't tried w/o this. It's possible this is changing the timing, but I
think
> > it only let the existing test scrape by with success due to faster timing.
The
> > test is failing because current time jumps from ~16ms to 2.1seconds
instantly
> > for some reason. I'm sending runs to the bots with extra logging to track
this
> > down.
> 
> Looks like MediaPlayerBridge is returning the duration as current time
> milliseconds after playback starts....
> ERROR:media_player_bridge.cc(519)] OnTimeUpdateTimerFired current_time:
2001000
> 
> I think 4.4.4 images may have problems playing these files... Rebuilding my
> emulator with the same image the bots use.

I can't get the emulator up, so I'll have to image a device tomorrow with 4.4.4.
In the meantime I'm adding mp3 versions of the ogg clips. It's possible these
test clips have never played on the bots before, but due to timing the tests are
still passing.

[DaleCurtis, 2016-01-27 02:25:13]

nasko@, timav@ PTAL. Thanks!

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:555:
AddPlayer(player);
On 2016/01/26 18:44:20, Tima Vaisburd wrote:
> On 2016/01/26 16:17:58, DaleCurtis wrote:
> > On 2016/01/26 07:10:14, Tima Vaisburd wrote:
> > > Do we need consistency between |players_| and
> |player_id_to_delegate_id_map_|
> > ?
> > > In this case should AddPlayer() be private?
> > 
> > Yes, that seems reasonable. I don't see anyone using it externally?
> 
> Neither do I.

Done.

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.cc (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:41: // the
BrowserMediaPlayerManagers.  Tracked by http://crbug.com/580626
On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> Maybe this comment needs some more explanations. Slow brains like mine might
> have a hard time to follow. Now that I understand, maybe something like:
> "WebMediaPlayerAndroid isn't aware of audio presence but MediaPlayerManager
> knows and will provide the information when calling RequestPlay() which
> ultimately will call ::Initialize(). Though, WebMediaPlayer will keep calling
> ::Initialize() with has_audio set to false so we assume has_audio is true if
it
> was once true which is the case when has_session_ is true."

Reworded. Let me know if it needs more work.

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.cc:69: // We already have
a session of the correct type, so do nothing more.
On 2016/01/26 21:35:08, DaleCurtis wrote:
> On 2016/01/26 21:23:33, DaleCurtis wrote:
> > On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > > I think this is might lead to bugs. `session_type_` might be the same as
> > > `media_session_type`. Actually, it should most likely be the same unless
the
> > > media's length changed. However, that doesn't mean that the
`media_session_`
> > > current type is `session_type_`.
> > > 
> > > BrowserMediaPlayerManager isn't trying to do much logic before calling
> > > `AddPlayer`. I think you should probably do the same here and call
> > `AddPlayer()`
> > > as often as possible and live it to it to decide if the type changed. The
> > thing
> > > changed a bit since it was designed. We used to assume that a player would
> be
> > > added once but I think calling `AddPlayer()` for an already added player
> > should
> > > have no side effect.
> > > 
> > > Maybe you should add a unit test for this? :)
> > 
> > I think we want to be careful about having duplicate players in the session;
> > otherwise we can get duplicate pause/play events which can frustrate users
if
> > they are delivered between a user interaction to the contrary.
> > 
> > I don't follow what's wrong with this code block. If we got to this point
> > there's already a MediaSession of the right type active (has_session_ must
be
> > true), no? So this should be a no-op other than the fact that it doesn't
> insert
> > duplicate player_ids into the MediaSession. Can you elaborate more on what's
> > wrong here? Also the MediaSessionController::Reinitialize() test should be
> > covering this -- I'll extend it to include anything we discuss here :)
> 
> Sorted out offline. MediaSession is a stack type data structure. So even if
the
> session is of the same type we need to move it back up the stack once playback
> starts.

Done.

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
File content/browser/media/android/media_session_controller.h (right):

https://codereview.chromium.org/1570043002/diff/260001/content/browser/media/...
content/browser/media/android/media_session_controller.h:15: class
MediaWebContentsObserver;
On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> nit: leave an empty line between `namespace` and `class`

Done.

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
On 2016/01/27 00:17:08, DaleCurtis wrote:
> On 2016/01/26 21:59:27, DaleCurtis wrote:
> > On 2016/01/26 21:23:33, DaleCurtis wrote:
> > > On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > > > I think the test fails because of this. We used to pass the test if
play()
> > was
> > > > successful. Did you try without this? or maybe with another file?
> > > 
> > > Haven't tried w/o this. It's possible this is changing the timing, but I
> think
> > > it only let the existing test scrape by with success due to faster timing.
> The
> > > test is failing because current time jumps from ~16ms to 2.1seconds
> instantly
> > > for some reason. I'm sending runs to the bots with extra logging to track
> this
> > > down.
> > 
> > Looks like MediaPlayerBridge is returning the duration as current time
> > milliseconds after playback starts....
> > ERROR:media_player_bridge.cc(519)] OnTimeUpdateTimerFired current_time:
> 2001000
> > 
> > I think 4.4.4 images may have problems playing these files... Rebuilding my
> > emulator with the same image the bots use.
> 
> I can't get the emulator up, so I'll have to image a device tomorrow with
4.4.4.
> In the meantime I'm adding mp3 versions of the ogg clips. It's possible these
> test clips have never played on the bots before, but due to timing the tests
are
> still passing.

Using mp3 clips causes the tests to pass, so I've added a secondary set of mp3
(but left the .ogg for when proprietary_codecs=false in the potential future). I
chose mp3 instead of mp4 since WMPA unfortunately does its detection based on
mime types.

[DaleCurtis, 2016-01-27 02:30:42]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[DaleCurtis, 2016-01-27 02:31:12]

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/1570043002/diff/260001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:102:
&& DOMUtils.getCurrentTime(webContents, id) > 0;
On 2016/01/27 02:25:13, DaleCurtis wrote:
> On 2016/01/27 00:17:08, DaleCurtis wrote:
> > On 2016/01/26 21:59:27, DaleCurtis wrote:
> > > On 2016/01/26 21:23:33, DaleCurtis wrote:
> > > > On 2016/01/26 21:02:50, Mounir Lamouri wrote:
> > > > > I think the test fails because of this. We used to pass the test if
> play()
> > > was
> > > > > successful. Did you try without this? or maybe with another file?
> > > > 
> > > > Haven't tried w/o this. It's possible this is changing the timing, but I
> > think
> > > > it only let the existing test scrape by with success due to faster
timing.
> > The
> > > > test is failing because current time jumps from ~16ms to 2.1seconds
> > instantly
> > > > for some reason. I'm sending runs to the bots with extra logging to
track
> > this
> > > > down.
> > > 
> > > Looks like MediaPlayerBridge is returning the duration as current time
> > > milliseconds after playback starts....
> > > ERROR:media_player_bridge.cc(519)] OnTimeUpdateTimerFired current_time:
> > 2001000
> > > 
> > > I think 4.4.4 images may have problems playing these files... Rebuilding
my
> > > emulator with the same image the bots use.
> > 
> > I can't get the emulator up, so I'll have to image a device tomorrow with
> 4.4.4.
> > In the meantime I'm adding mp3 versions of the ogg clips. It's possible
these
> > test clips have never played on the bots before, but due to timing the tests
> are
> > still passing.
> 
> Using mp3 clips causes the tests to pass, so I've added a secondary set of mp3
> (but left the .ogg for when proprietary_codecs=false in the potential future).
I
> chose mp3 instead of mp4 since WMPA unfortunately does its detection based on
> mime types.

I should mention I validated the ogg files with oggz-validate and they appear
correct, so I'm not sure why Android 4.4.4 (as used by the bots) can't play
these files. I'm unable to get an emulator image with this working locally,
though I will try to image a device tomorrow for further investigations.

[Tima Vaisburd, 2016-01-27 02:43:07]

MediaPlayerManager lgtm

[nasko, 2016-01-27 18:19:55]

LGTM

https://codereview.chromium.org/1570043002/diff/280001/content/browser/media/...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1570043002/diff/280001/content/browser/media/...
content/browser/media/media_web_contents_observer.h:58: void
OnMediaPaused(RenderFrameHost* render_frame_host,
nit: Order would be nice to match the IPC file, which will also make it
alphabetical.

[DaleCurtis, 2016-01-27 19:37:27]

Thanks for reviews everyone!

https://codereview.chromium.org/1570043002/diff/280001/content/browser/media/...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1570043002/diff/280001/content/browser/media/...
content/browser/media/media_web_contents_observer.h:58: void
OnMediaPaused(RenderFrameHost* render_frame_host,
On 2016/01/27 18:19:55, nasko wrote:
> nit: Order would be nice to match the IPC file, which will also make it
> alphabetical.

Done.

[DaleCurtis, 2016-01-27 19:37:35]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-01-27 19:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, mlamouri@chromium.org, timav@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1570043002/#ps300001
(title: "Reorder.")

[commit-bot: I haz the power, 2016-01-27 19:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570043002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570043002/300001

[commit-bot: I haz the power, 2016-01-27 21:10:32]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

[commit-bot: I haz the power, 2016-01-27 21:10:33]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2016-01-27 21:11:31]

Description was changed from

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.
==========

to

==========
Implement MediaSession on top of the WebMediaPlayerDelegate.

Extracts the existing MediaSession usage from the browser media
player manager and moves it into the WebMediaPlayerDelegate where
it can be shared by both the desktop and android media players.

Notes:
- Removes RenderFrameObserver from all WebMediaPlayers since the
delegate observer interface handles all of their needs.
- The MediaStream WebMediaPlayer now has MediaSession support.
- Currently the WMPI has no concept of requesting permission to
play, the WMPA behavior has been preserved via a request to the
MediaWebContentsObserverAndroid from BrowserMediaPlayerManager.
- Fixes flakiness issues with the MediaSession tests which were
not completely waiting for playback to start before moving on
with test expectations.
- Extracts misplaced delegate messages from frame_messages.h and
puts them in their own media_player_delegate_messages.h file.
- During the message move, cleans up the player_cookie (a int64_t
pointer) in favor of a plain int. Renames messages for the better.
- Removes all delegate calls from the cast adapter since they are
always remote type which should be ignored anyways.
- |has_audio| is sticky in the MediaSessionController since WMPA
can't be relied upon to provide a true value and plumbing the
value from every MediaPlayerAndroid is non-trivial.
- Fixes MediaSession ContentShellTests which were passing on the
bots by happy circumstance -- the bots do not appear able to play
the .ogg files checked in -- instead instantly ending. Test were fixed
by adding .mp3 variants.

BUG=529887, 580626
TEST=new tests, existing MediaSession tests pass with and without
the unified media pipeline flag, manual verification of background
behavior.

Committed: https://crrev.com/bb3eaacc70007d44dc7788fcbbe106977f49b066
Cr-Commit-Position: refs/heads/master@{#371870}
==========

[commit-bot: I haz the power, 2016-01-27 21:11:32]

Patchset 9 (id:??) landed as
https://crrev.com/bb3eaacc70007d44dc7788fcbbe106977f49b066
Cr-Commit-Position: refs/heads/master@{#371870}