Move audio focus control from media/ to content/ and make it per WebContents.

Move audio focus control from media/ to content/ and make it per WebContents.

This CL is making each WebContents able to grab its own audio focus from the
system instead of keeping the audio focus for Chromium regardless of the number
of tab producing noise.

This is improving Chromium integration in Android by having each tab behave as
a user would expect an application to behave. For example, it is now possible
to go to a music website, play something then go to Youtube and play a video.
The video will automatically pause the music, the same way the native Youtube
application would pause a music application.

There is a slight "hack" where for sounds known to be short, the audio focus
is requested as TRANSIENT_MAY_DUCK in order to not break running media for
sound effects.

BUG=486878
Committed: https://crrev.com/3d020b1e04e93bbd318105276a89dd3aa4af1a27
Cr-Commit-Position: refs/heads/master@{#331947}

[mlamouri (slow - plz ping), 2015-05-11 20:20:02]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, qinmin@chromium.org, tedchoc@chromium.org

[whywhat, 2015-05-12 12:50:47]

I believe this needs more testing (e.g. remote players, multi window scenarios)
and some tests added/updated :)
Overall implementation lgtm with numerous nits.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:164:
MediaSession::Get(web_contents())->AbandonAudioFocus(this);
nit: s/Get/From?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:349:
MediaPlayerAndroid* player = GetPlayer(player_id);
I believe this should return true for remote players or remote players shouldn't
call this method.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:352:
player->GetDuration().InSeconds() > 5 ? MediaSession::Type::Content
5 is a magic constant! make it a named one, please.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:8: #include
"content/browser/media/android/browser_media_player_manager.h"
You don't need this include.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:46: :
WebContentsObserver(web_contents)
follow the style guide here that's different from Blink's one:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_...

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:58: if (has_audio_focus_ &&
if you had None in Type as the first member, could be simply if (type <=
audio_focus_type_)
not sure if all audio focus types would be sorted by strength.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:80: for
(PlayersMap::Iterator<PlayerIdentifier> iter(&players_); !iter.IsAtEnd();
why using IDMap if you're not using its ids?
something like hash_map<Delegate*, hash_set<int>> sounds like a better choice
from the first glance.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:19: // MediaSession manages the
Android AudioFocus for a given WebContents. It is
I believe a more expanded comment would be welcome here.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:35: bool
RequestAudioFocus(MediaSessionDelegate* delegate,
Document the public methods.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:36: int player_id,
This sounds more like "EnsureSufficientAudioFocus" because it only requests it
under some circumstances.
Or even CanPlayerPlay() to avoid exposing the audio focus to the users of the
class?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:38: void
AbandonAudioFocus(MediaSessionDelegate* delegate,
Could be OnPlayerStopped then if you accept the suggestion above?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:40: void
AbandonAudioFocus(MediaSessionDelegate* delegate);
Ditto. OnAllPlayersStopped() maybe?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:56:
DISALLOW_COPY_AND_ASSIGN(PlayerIdentifier);
nit: is it common for structs? not sure if this is common.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:67: void
AbandonAudioFocusIfNeeded();
s/AbandonAudioFocus/AbandonSystemAudioFocus?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:72: bool has_audio_focus_;
Add None to Type and just have one three-state member?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session_delegate.h (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session_delegate.h:11: #include
"base/id_map.h"
nit: IWYU - I don't think you need any of those includes here.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session_delegate.h:25: // The given
|player_id| has been suspended by the MediaSession.
nit: s/suspended/resumed

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSession.java
(right):

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:14:
* MediaSession is the Java counterpart of content::MediaSession.
I believe more comment is due. E.g. explain the fact that it maintains the
"strongest" audio focus type needed at the moment and keeps the current focus
type acquired in mFocusType.

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:23:
private long mNativeMediaSession;
make it final.
have you made it certain that the C++ counterpart outlives the Java one (I
believe by holding the Java object in a strong reference)

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:55:
public void onAudioFocusChange(int focusChange) {
@Override?

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:58:
if (requestAudioFocusInternal()) {
nit:
does it all fit in one line?
{} are not needed in any case

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
media/base/android/media_player_manager.h:86: virtual bool RequestPlay(int
player_id) = 0;
nit: s/RequestPlay/{CanPlay|IsAllowedToPlay}?

[whywhat, 2015-05-12 12:50:47]

I believe this needs more testing (e.g. remote players, multi window scenarios)
and some tests added/updated :)
Overall implementation lgtm with numerous nits.

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
media/base/android/media_player_manager.h:86: virtual bool RequestPlay(int
player_id) = 0;
nit: s/RequestPlay/{CanPlay|IsAllowedToPlay}?

[qinmin, 2015-05-15 18:06:20]

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:274: void
BrowserMediaPlayerManager::OnError(int player_id, int error) {
what should happen to the onError case? We still hold the audio focus?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:352:
player->GetDuration().InSeconds() > 5 ? MediaSession::Type::Content
You need to special treat duration==0 case. Duration is not available for HLS or
if wifi is not available.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:22: class MediaSession : public
content::WebContentsObserver,
Can we merge this class with MediaWebContentsObserver? They are both observing
all media elements inside a webcontent, and holding a map of manager or player.

[mlamouri (slow - plz ping), 2015-05-19 21:56:17]

Review comments applied. PTAL.

Will write some tests tomorrow.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:164:
MediaSession::Get(web_contents())->AbandonAudioFocus(this);
On 2015/05/12 at 12:50:44, whywhat wrote:
> nit: s/Get/From?

I used ::Get() instead of ::From() because ::FromWebContents() is used to return
a WebContentsUserData but wouldn't create it by default. ::Get() will create the
instance if needed, like a singleton pattern.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:349:
MediaPlayerAndroid* player = GetPlayer(player_id);
On 2015/05/12 at 12:50:45, whywhat wrote:
> I believe this should return true for remote players or remote players
shouldn't call this method.

Remote players do not call this method AFAICT. However, they grab audiofocus in
their own way from the lockscreen code.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:352:
player->GetDuration().InSeconds() > 5 ? MediaSession::Type::Content
> 5 is a magic constant! make it a named one, please.

Oups again.

> You need to special treat duration==0 case. Duration is not available for HLS
or if wifi is not available.

It seems that we don't get duration==0 for HLS. Also, ::RequestPlay() is called
when the data is loaded and ready to be played so if the wifi isn't available,
this will not be called. Could that be the reason why duration!=0 for HLS?

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:8: #include
"content/browser/media/android/browser_media_player_manager.h"
On 2015/05/12 at 12:50:45, whywhat wrote:
> You don't need this include.

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:46: :
WebContentsObserver(web_contents)
On 2015/05/12 at 12:50:45, whywhat wrote:
> follow the style guide here that's different from Blink's one:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_...

Sorry.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:58: if (has_audio_focus_ &&
On 2015/05/12 at 12:50:45, whywhat wrote:
> if you had None in Type as the first member, could be simply if (type <=
audio_focus_type_)
> not sure if all audio focus types would be sorted by strength.

I wouldn't recommend doing that. I could write a helper method like
IsCurrentAudioFocusSufficient() if you think it would be better.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:80: for
(PlayersMap::Iterator<PlayerIdentifier> iter(&players_); !iter.IsAtEnd();
On 2015/05/12 at 12:50:45, whywhat wrote:
> why using IDMap if you're not using its ids?
> something like hash_map<Delegate*, hash_set<int>> sounds like a better choice
from the first glance.

Done. I'm using a hash_set now. It avoids adding duplicates too.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:19: // MediaSession manages the
Android AudioFocus for a given WebContents. It is
On 2015/05/12 at 12:50:45, whywhat wrote:
> I believe a more expanded comment would be welcome here.

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:22: class MediaSession : public
content::WebContentsObserver,
On 2015/05/15 at 18:06:19, qinmin wrote:
> Can we merge this class with MediaWebContentsObserver? They are both observing
all media elements inside a webcontent, and holding a map of manager or player.

They are not quite doing the same thing. MediaSession has a map of active
players, part of the session. MediaWebContentsObserver has a map of
MediaPlayerManagers associated to each RenderFrameHost in a WebContents. Merging
them will be very odd and might just create a big class that would do two
different things.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:35: bool
RequestAudioFocus(MediaSessionDelegate* delegate,
On 2015/05/12 at 12:50:45, whywhat wrote:
> Document the public methods.

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:36: int player_id,
On 2015/05/12 at 12:50:45, whywhat wrote:
> This sounds more like "EnsureSufficientAudioFocus" because it only requests it
under some circumstances.
> Or even CanPlayerPlay() to avoid exposing the audio focus to the users of the
class?

Renamed AddPlayer(). I changed how things behave to not add to the players_ list
if the audio focus isn't granted.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:38: void
AbandonAudioFocus(MediaSessionDelegate* delegate,
On 2015/05/12 at 12:50:46, whywhat wrote:
> Could be OnPlayerStopped then if you accept the suggestion above?

Renamed RemovePlayer(). OnPlayerStopped() is confusing given that we call it in
OnPause(). RemovePlayer() makes it clear that the player is removed from the
session.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:40: void
AbandonAudioFocus(MediaSessionDelegate* delegate);
On 2015/05/12 at 12:50:46, whywhat wrote:
> Ditto. OnAllPlayersStopped() maybe?

RemovePlayers()

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:40: void
AbandonAudioFocus(MediaSessionDelegate* delegate);
On 2015/05/12 at 12:50:46, whywhat wrote:
> Ditto. OnAllPlayersStopped() maybe?

RemovePlayers()

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:56:
DISALLOW_COPY_AND_ASSIGN(PlayerIdentifier);
On 2015/05/12 at 12:50:46, whywhat wrote:
> nit: is it common for structs? not sure if this is common.

AFAIK, it is recommended if not required.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:67: void
AbandonAudioFocusIfNeeded();
On 2015/05/12 at 12:50:45, whywhat wrote:
> s/AbandonAudioFocus/AbandonSystemAudioFocus?

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.h:72: bool has_audio_focus_;
On 2015/05/12 at 12:50:46, whywhat wrote:
> Add None to Type and just have one three-state member?

When ::OnResume() is called, the boolean is toggled back to true. To have the
same behaviour with a tree state, we would need to save the previous value,
which would be actually more expensive memory-wise.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session_delegate.h (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session_delegate.h:11: #include
"base/id_map.h"
On 2015/05/12 at 12:50:46, whywhat wrote:
> nit: IWYU - I don't think you need any of those includes here.

Oups :)

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/media_session_delegate.h:25: // The given
|player_id| has been suspended by the MediaSession.
On 2015/05/12 at 12:50:46, whywhat wrote:
> nit: s/suspended/resumed

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSession.java
(right):

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:14:
* MediaSession is the Java counterpart of content::MediaSession.
On 2015/05/12 at 12:50:46, whywhat wrote:
> I believe more comment is due. E.g. explain the fact that it maintains the
"strongest" audio focus type needed at the moment and keeps the current focus
type acquired in mFocusType.

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:23:
private long mNativeMediaSession;
On 2015/05/12 at 12:50:46, whywhat wrote:
> make it final.
> have you made it certain that the C++ counterpart outlives the Java one (I
believe by holding the Java object in a strong reference)

Yes, the java object is held as ScopedJavaGlobalRef.

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:55:
public void onAudioFocusChange(int focusChange) {
On 2015/05/12 at 12:50:46, whywhat wrote:
> @Override?

Done.

https://codereview.chromium.org/1110833004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:58:
if (requestAudioFocusInternal()) {
On 2015/05/12 at 12:50:46, whywhat wrote:
> nit:
> does it all fit in one line?
> {} are not needed in any case

Chromium is using Android coding style in Java. What I wrote is allowed. Keeping
it in one line is allowed. Removing the braces isn't:
http://source.android.com/source/code-style.html

I prefer to keep it as-is given that it is allowed per coding style and having
it in one line is... unusual ;)

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/1110833004/diff/20001/media/base/android/medi...
media/base/android/media_player_manager.h:86: virtual bool RequestPlay(int
player_id) = 0;
On 2015/05/12 at 12:50:47, whywhat wrote:
> nit: s/RequestPlay/{CanPlay|IsAllowedToPlay}?

I used ::RequestPlay() because it's not just returning a boolean, it's doing
things. CanPlay() or IsAllowedToPlay() might give the wrong impression that this
is just going to return true/false with no side effects.

[qinmin, 2015-05-20 21:35:25]

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:352:
player->GetDuration().InSeconds() > 5 ? MediaSession::Type::Content
On 2015/05/19 21:56:15, Mounir Lamouri wrote:
> > 5 is a magic constant! make it a named one, please.
> 
> Oups again.
> 
> > You need to special treat duration==0 case. Duration is not available for
HLS
> or if wifi is not available.
> 
> It seems that we don't get duration==0 for HLS. Also, ::RequestPlay() is
called
> when the data is loaded and ready to be played so if the wifi isn't available,
> this will not be called. Could that be the reason why duration!=0 for HLS?

HLS duration is never trustworthy on android. If wifi isn't available, this will
also be called if the device is on mobile networks. But since startInternal is
always called after prepare(), so the player should have the duration here.

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

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:362:
OnSuspend(player_id);
MediaPlayerAndroid::RequestPlay() could possibly call
MediaPlayerAndroid::Pause(), the reentrance makes the code very fragile to
errors.
And the logic is also wierd, if RequestPlay() returns false, shouldn't the
player handle the failed start() call by itself? Rather than letting the manager
calling pause for it.

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:112: players_.erase(it++);
break?

[mlamouri (slow - plz ping), 2015-05-22 15:58:53]

PTAL. This CL does contain tests.

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1110833004/diff/20001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:352:
player->GetDuration().InSeconds() > 5 ? MediaSession::Type::Content
On 2015/05/20 at 21:35:24, qinmin wrote:
> On 2015/05/19 21:56:15, Mounir Lamouri wrote:
> > > 5 is a magic constant! make it a named one, please.
> > 
> > Oups again.
> > 
> > > You need to special treat duration==0 case. Duration is not available for
HLS
> > or if wifi is not available.
> > 
> > It seems that we don't get duration==0 for HLS. Also, ::RequestPlay() is
called
> > when the data is loaded and ready to be played so if the wifi isn't
available,
> > this will not be called. Could that be the reason why duration!=0 for HLS?
> 
> HLS duration is never trustworthy on android. If wifi isn't available, this
will also be called if the device is on mobile networks. But since startInternal
is always called after prepare(), so the player should have the duration here.

Yeah, we made the call in StartInternal() on purpose so we don't cut other audio
if the player ends up not playable.

My understanding is that there is nothing to change here, let me know if I'm
misunderstanding.

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

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:362:
OnSuspend(player_id);
On 2015/05/20 at 21:35:24, qinmin wrote:
> MediaPlayerAndroid::RequestPlay() could possibly call
MediaPlayerAndroid::Pause(), the reentrance makes the code very fragile to
errors.
> And the logic is also wierd, if RequestPlay() returns false, shouldn't the
player handle the failed start() call by itself? Rather than letting the manager
calling pause for it.

It's something I actually fixed while writing unit tests for the player.

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:112: players_.erase(it++);
On 2015/05/20 at 21:35:25, qinmin wrote:
> break?

There might be more than one player.

[mlamouri (slow - plz ping), 2015-05-22 16:00:16]

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

[mlamouri (slow - plz ping), 2015-05-22 16:00:16]

+torne@ for android_webview/

[Torne, 2015-05-22 16:23:54]

android_webview LGTM

[qinmin, 2015-05-22 16:43:23]

https://codereview.chromium.org/1110833004/diff/80001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:83: // The session should be
reset if a player is starting while all players are
I am not sure whether the logic is appropriate. Say if i have several players
playing, and then a transient audio loss happens, so all the players become
paused.
Now if a new player starts playing, we will remove all the transiently paused
players.
And what if the player fails to request AudioFocus in the next couple lines? And
when the transient AudioFocus loss is finished, OnResume() will find players_ is
empty.

[mlamouri (slow - plz ping), 2015-05-22 19:01:59]

PTAL

https://codereview.chromium.org/1110833004/diff/80001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:83: // The session should be
reset if a player is starting while all players are
On 2015/05/22 at 16:43:23, qinmin wrote:
> I am not sure whether the logic is appropriate. Say if i have several players
playing, and then a transient audio loss happens, so all the players become
paused.
> Now if a new player starts playing, we will remove all the transiently paused
players.
> And what if the player fails to request AudioFocus in the next couple lines?
And when the transient AudioFocus loss is finished, OnResume() will find
players_ is empty.

You're right, we should make sure the audio focus request was successful before
clearing the list. However, I'm not sure if we should avoid playing a sound
because a transient sound is playing. It could be an unexpected behaviour for
the user. Then, resuming the entire session might be unexpected too.

[qinmin, 2015-05-23 18:16:06]

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:42: // Returns the MediaSession
associated to this WebContents. Create one if none
nit:s/Create/Creates/

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:53: // Removes the given player
from the current media session. Abandon audio
s/Abandon/Abandons/

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:57: // Removes all the players
associated with |observer|. Abandon audio focus if
ditto

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

https://codereview.chromium.org/1110833004/diff/100001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:25:
* Plays the media with given {@code id}.
nit: missing @param here and below

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
media/base/android/media_player_bridge.cc:460: Pause(true);
this should be false, see my comments in media_source_player.cc

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
media/base/android/media_source_player.cc:221: Pause(true);
i think this should be false, the value is use by remote_media_player_bridge.
If the local player loses AudioFocus, it shouldn't impact the remote player.

[mlamouri (slow - plz ping), 2015-05-23 20:05:28]

PTAL

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:42: // Returns the MediaSession
associated to this WebContents. Create one if none
On 2015/05/23 at 18:16:06, qinmin wrote:
> nit:s/Create/Creates/

Done.

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:53: // Removes the given player
from the current media session. Abandon audio
On 2015/05/23 at 18:16:06, qinmin wrote:
> s/Abandon/Abandons/

Done.

https://codereview.chromium.org/1110833004/diff/100001/content/browser/media/...
content/browser/media/android/media_session.h:57: // Removes all the players
associated with |observer|. Abandon audio focus if
On 2015/05/23 at 18:16:06, qinmin wrote:
> ditto

Done.

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

https://codereview.chromium.org/1110833004/diff/100001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:25:
* Plays the media with given {@code id}.
On 2015/05/23 at 18:16:06, qinmin wrote:
> nit: missing @param here and below

I filed https://crbug.com/491588 for this. @param is never used in that file, I
will be happy to write a CL improving javadoc in there.

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1110833004/diff/100001/media/base/android/med...
media/base/android/media_source_player.cc:221: Pause(true);
On 2015/05/23 at 18:16:06, qinmin wrote:
> i think this should be false, the value is use by remote_media_player_bridge.
> If the local player loses AudioFocus, it shouldn't impact the remote player.

This question might sound silly but what's the link between this and
RemoteMediaPlayerBridge? I wasn't able to find why passing true or false would
have any impact there.

[mlamouri (slow - plz ping), 2015-05-24 17:09:43]

On 2015/05/23 at 20:05:28, Mounir Lamouri wrote:
>
https://codereview.chromium.org/1110833004/diff/100001/content/public/test/an...
>
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:25:
* Plays the media with given {@code id}.
> On 2015/05/23 at 18:16:06, qinmin wrote:
> > nit: missing @param here and below
> 
> I filed https://crbug.com/491588 for this. @param is never used in that file,
I will be happy to write a CL improving javadoc in there.

See https://codereview.chromium.org/1154913003

[qinmin, 2015-05-24 21:33:53]

https://codereview.chromium.org/1110833004/diff/120001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java
(right):

https://codereview.chromium.org/1110833004/diff/120001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:161:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_LOSS));
From the trybots, the test is failing on this line and so do many other tests.
Do you have any idea why they are failing?

[mlamouri (slow - plz ping), 2015-05-26 14:08:02]

I solved the trybot issue. It appears that I was using media files that my Nexus
4 was able to read but not the Nexus 5. I thought the bots were still using
Nexus 4. I tried these tests on N4 and N5 and they were fine.

I also use the opportunity to de-flake a bit the tests.

tedchoc@ and qinmin@, PTAL.

[Ted C, 2015-05-27 00:59:09]

some general comments, but I will certainly defer to Min's final approval on the
change as I am no media guru

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:55: void
MediaSession::Initialize() {
the ordering of methods should match the header

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:110: for (auto it =
players_.begin(); it != players_.end(); ) {
extra space after the last ;

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:121: if
(j_media_session_.is_null())
how could this be null?  initialize is called after being constructed and the
object only goes away when this class dies.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:131: if (!has_audio_focus_ ||
!players_.empty())
what happens if we had previously got a OnSuspend from java (like the
AUDIOFOCUS_LOSS_TRANSIENT case) and in the mean time we remove all the players. 
In that case, we never call abandonAudioFocus.

Does it happen through some other means?

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:153: 
unnecessary extra blank line

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.h:30: // It is communicating to the
Android system trough MediaSession.java via JNI.
This seems like more of an implementation detail that I would leave out.

If anything, I would say that "Android system interaction occurs in the java
counterpart to this class".  Putting the exact class name is something a
consumer of this class doesn't need (and honestly the fact there is a java
counterpart just isn't needed either).

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.h:61: // Called by Java trough JNI.
through.

And why are these called?  Because android has requested that this audio should
be suspended?  Is it because another audio source has taken precedence?

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/MediaSession.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:47:
: AudioManager.AUDIOFOCUS_GAIN;
java is 8 from the start of the previous line

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:51:
throw new Exception("Did not get audio focus!");
I would just do:

 fail("Did not get audio focus");

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:71:
int mAudioFocusState = AudioManager.AUDIOFOCUS_LOSS;
this should go to the top of this inner class

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:115:
assertFalse(DOMUtils.isMediaPaused(getWebContents(), SHORT_AUDIO));
how short are these short audio and video clips?  Could this ever be too slow
that it would be finished now?

Just wondering if we should check paused or finished I guess.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:133:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_GAIN));
how long does this transition take from DUCK to GAIN?

I'm worried these are going to be really slow tests.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:161:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_LOSS));
this is ensuring that the mock audio focus loses focus because it was taken over
by running the long audio?  Just making sure I know what is going on here.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:221:
// Wait for the media to be really playing.
why this comment and not in the ones above that do the same thing?

[mlamouri (slow - plz ping), 2015-05-28 16:20:41]

Review comments applied. PTAL.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:55: void
MediaSession::Initialize() {
On 2015/05/27 at 00:59:08, Ted C wrote:
> the ordering of methods should match the header

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:110: for (auto it =
players_.begin(); it != players_.end(); ) {
On 2015/05/27 at 00:59:08, Ted C wrote:
> extra space after the last ;

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:121: if
(j_media_session_.is_null())
On 2015/05/27 at 00:59:08, Ted C wrote:
> how could this be null?  initialize is called after being constructed and the
object only goes away when this class dies.

It is reset by content_browsertests to cut down the Android bits. I agree that
it's not ideal but that's the best way I found to be able to test this class
without being affected by Android API side effects.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:131: if (!has_audio_focus_ ||
!players_.empty())
On 2015/05/27 at 00:59:08, Ted C wrote:
> what happens if we had previously got a OnSuspend from java (like the
AUDIOFOCUS_LOSS_TRANSIENT case) and in the mean time we remove all the players. 
In that case, we never call abandonAudioFocus.
> 
> Does it happen through some other means?

Good catch. Should be fixed.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:153: 
On 2015/05/27 at 00:59:08, Ted C wrote:
> unnecessary extra blank line

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:153: 
On 2015/05/27 at 00:59:08, Ted C wrote:
> unnecessary extra blank line

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.h:30: // It is communicating to the
Android system trough MediaSession.java via JNI.
On 2015/05/27 at 00:59:08, Ted C wrote:
> This seems like more of an implementation detail that I would leave out.
> 
> If anything, I would say that "Android system interaction occurs in the java
counterpart to this class".  Putting the exact class name is something a
consumer of this class doesn't need (and honestly the fact there is a java
counterpart just isn't needed either).

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.h:61: // Called by Java trough JNI.
On 2015/05/27 at 00:59:08, Ted C wrote:
> through.
> 
> And why are these called?  Because android has requested that this audio
should be suspended?  Is it because another audio source has taken precedence?

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/MediaSession.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/MediaSession.java:47:
: AudioManager.AUDIOFOCUS_GAIN;
On 2015/05/27 at 00:59:08, Ted C wrote:
> java is 8 from the start of the previous line

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:51:
throw new Exception("Did not get audio focus!");
On 2015/05/27 at 00:59:09, Ted C wrote:
> I would just do:
> 
>  fail("Did not get audio focus");

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:71:
int mAudioFocusState = AudioManager.AUDIOFOCUS_LOSS;
On 2015/05/27 at 00:59:08, Ted C wrote:
> this should go to the top of this inner class

Done.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:115:
assertFalse(DOMUtils.isMediaPaused(getWebContents(), SHORT_AUDIO));
On 2015/05/27 at 00:59:09, Ted C wrote:
> how short are these short audio and video clips?  Could this ever be too slow
that it would be finished now?
> 
> Just wondering if we should check paused or finished I guess.

The long ones are 6 seconds long. The short ones are 2 seconds long. If one of
them finishes while this test is running, we might have bigger problems ;)

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:133:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_GAIN));
On 2015/05/27 at 00:59:08, Ted C wrote:
> how long does this transition take from DUCK to GAIN?
> 
> I'm worried these are going to be really slow tests.

2 second. WDYT? I could have a VERY_SHORT_AUDIO that is 1 second long for that
specific test.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:161:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_LOSS));
On 2015/05/27 at 00:59:08, Ted C wrote:
> this is ensuring that the mock audio focus loses focus because it was taken
over by running the long audio?  Just making sure I know what is going on here.

Correct.

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:221:
// Wait for the media to be really playing.
On 2015/05/27 at 00:59:09, Ted C wrote:
> why this comment and not in the ones above that do the same thing?

Because here we don't really care about the value. It could be replaced by
IsMediaReallyPlaying(). Above, we actually care about the value, because we want
to check that the focus state changes from LOSS_TRANSIENT_CAN_DUCK to
LOSS_TRANSIENT.

Note: waitForMediaPlay() is checking the media element property, not that the
media is actually playing something, which is when the audio focus request kicks
in.

[qinmin, 2015-05-28 17:46:04]

lgtm

[Ted C, 2015-05-28 20:46:45]

lgtm

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:110: for (auto it =
players_.begin(); it != players_.end(); ) {
On 2015/05/28 16:20:39, Mounir Lamouri wrote:
> On 2015/05/27 at 00:59:08, Ted C wrote:
> > extra space after the last ;
> 
> Done.

I meant that you didn't need the extra space, now there are two :-)

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:133:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_GAIN));
On 2015/05/28 16:20:40, Mounir Lamouri wrote:
> On 2015/05/27 at 00:59:08, Ted C wrote:
> > how long does this transition take from DUCK to GAIN?
> > 
> > I'm worried these are going to be really slow tests.
> 
> 2 second. WDYT? I could have a VERY_SHORT_AUDIO that is 1 second long for that
> specific test.

If it's easy, go ahead and add a short one to make this faster.  We need to make
all of our tests faster now that we want to run more on the upstream CQ so
adding new long tests isn't going to help us meet that goal.

[mlamouri (slow - plz ping), 2015-05-29 09:30:03]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-05-29 09:30:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, torne@chromium.org, tedchoc@chromium.org,
qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1110833004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-05-29 09:30:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1110833004/200001

[commit-bot: I haz the power, 2015-05-29 10:04:01]

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

[commit-bot: I haz the power, 2015-05-29 10:04:04]

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

[mlamouri (slow - plz ping), 2015-05-29 11:14:13]

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1110833004/diff/160001/content/browser/media/...
content/browser/media/android/media_session.cc:110: for (auto it =
players_.begin(); it != players_.end(); ) {
On 2015/05/28 at 20:46:45, Ted C wrote:
> On 2015/05/28 16:20:39, Mounir Lamouri wrote:
> > On 2015/05/27 at 00:59:08, Ted C wrote:
> > > extra space after the last ;
> > 
> > Done.
> 
> I meant that you didn't need the extra space, now there are two :-)

OK. Fixed :)

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java
(right):

https://codereview.chromium.org/1110833004/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/MediaSessionTest.java:133:
assertTrue(mAudioFocusChangeListener.waitForFocusStateChange(AudioManager.AUDIOFOCUS_GAIN));
On 2015/05/28 at 20:46:45, Ted C wrote:
> On 2015/05/28 16:20:40, Mounir Lamouri wrote:
> > On 2015/05/27 at 00:59:08, Ted C wrote:
> > > how long does this transition take from DUCK to GAIN?
> > > 
> > > I'm worried these are going to be really slow tests.
> > 
> > 2 second. WDYT? I could have a VERY_SHORT_AUDIO that is 1 second long for
that
> > specific test.
> 
> If it's easy, go ahead and add a short one to make this faster.  We need to
make all of our tests faster now that we want to run more on the upstream CQ so
adding new long tests isn't going to help us meet that goal.

Done.

[mlamouri (slow - plz ping), 2015-05-29 11:27:26]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-05-29 11:27:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1110833004/200001

[commit-bot: I haz the power, 2015-05-29 12:34:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-29 12:35:19]

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