Free (and pause) audio resources when getting a phone call

Free (and pause) audio resources when getting a phone call


BUG=121898
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169922

[qinmin, 2012-11-23 23:46:38]

https://codereview.chromium.org/11348199/diff/7001/.gitmodules
File .gitmodules (left):

https://codereview.chromium.org/11348199/diff/7001/.gitmodules#oldcode2
.gitmodules:2: path = breakpad/src
Why is this file included in this change?

https://codereview.chromium.org/11348199/diff/7001/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11348199/diff/7001/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:226:
OnReleaseResources(player_id);
This will cause some problems on the webkit side mediaplayer state. The webkit
side is still thinking that the mediaplayer is playing, but it is actually
paused after this.
We need to send a MediaPlayerMsg_DidMediaPlayerPause msg to inform the webkit
about this.

https://codereview.chromium.org/11348199/diff/7001/content/shell/android/java...
File content/shell/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/11348199/diff/7001/content/shell/android/java...
content/shell/android/java/AndroidManifest.xml:69: <uses-permission
android:name="android.permission.READ_PHONE_STATE"/>
you might also need to add this to test shell

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(left):

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:19:
MediaPlayer.OnCompletionListener,
move these lines to align with the previous line

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:118:
(TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE);
fix indent, 8 chars instead of 4

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:141:
(TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE);
fix indent

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:143:
mgr.listen(listener, PhoneStateListener.LISTEN_CALL_STATE);
indent is 4

[Miguel Garcia, 2012-11-26 10:37:50]

Thanks for the quick review!

https://codereview.chromium.org/11348199/diff/7001/.gitmodules
File .gitmodules (left):

https://codereview.chromium.org/11348199/diff/7001/.gitmodules#oldcode2
.gitmodules:2: path = breakpad/src
On 2012/11/23 23:46:38, qinmin wrote:
> Why is this file included in this change?
I must have come in through a bad merge, I am removing it now.

https://codereview.chromium.org/11348199/diff/7001/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11348199/diff/7001/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:226:
OnReleaseResources(player_id);
Wow thanks a lot for catching this! I added the change and tested it by going to


http://romaindillet.github.com/That-Radio-Player/#

Selecting one of the radios to play, then call myself
The music stops and the display switches back to pause, it does seem WebKit gets
the message and notifies the UI.


On 2012/11/23 23:46:38, qinmin wrote:
> This will cause some problems on the webkit side mediaplayer state. The webkit
> side is still thinking that the mediaplayer is playing, but it is actually
> paused after this.
> We need to send a MediaPlayerMsg_DidMediaPlayerPause msg to inform the webkit
> about this.

https://codereview.chromium.org/11348199/diff/7001/content/shell/android/java...
File content/shell/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/11348199/diff/7001/content/shell/android/java...
content/shell/android/java/AndroidManifest.xml:69: <uses-permission
android:name="android.permission.READ_PHONE_STATE"/>
On 2012/11/23 23:46:38, qinmin wrote:
> you might also need to add this to test shell

Done.

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(left):

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:19:
MediaPlayer.OnCompletionListener,
On 2012/11/23 23:46:38, qinmin wrote:
> move these lines to align with the previous line

Moved them to 4 space indent, I think this is more compact. If you prefer
another alignment let me know of course.

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:118:
(TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE);
On 2012/11/23 23:46:38, qinmin wrote:
> fix indent, 8 chars instead of 4

Done.

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:141:
(TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE);
On 2012/11/23 23:46:38, qinmin wrote:
> fix indent

Done.

https://codereview.chromium.org/11348199/diff/7001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:143:
mgr.listen(listener, PhoneStateListener.LISTEN_CALL_STATE);
On 2012/11/23 23:46:38, qinmin wrote:
> indent is 4

Done.

[qinmin, 2012-11-26 15:02:20]

https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:228: routing_id(),
fullscreen_player_id_));
should be player_id, or otherwise, it will be routed to a different
webmediaplayer

[Miguel Garcia, 2012-11-26 15:42:28]

https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:228: routing_id(),
fullscreen_player_id_));
Thanks for noticing, changed now.

On 2012/11/26 15:02:21, qinmin wrote:
> should be player_id, or otherwise, it will be routed to a different
> webmediaplayer

[qinmin, 2012-11-26 18:48:41]

lgtm

[commit-bot: I haz the power, 2012-11-26 18:58:02]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Miguel Garcia, 2012-11-26 19:07:49]

Aaron, Yaron can you have a look? I need Owners approval in the different
directories.

Thanks!

On 2012/11/26 15:42:28, Miguel Garcia wrote:
>
https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
> File content/browser/android/media_player_manager_android.cc (right):
> 
>
https://codereview.chromium.org/11348199/diff/5012/content/browser/android/me...
> content/browser/android/media_player_manager_android.cc:228: routing_id(),
> fullscreen_player_id_));
> Thanks for noticing, changed now.
> 
> On 2012/11/26 15:02:21, qinmin wrote:
> > should be player_id, or otherwise, it will be routed to a different
> > webmediaplayer

[Yaron, 2012-11-27 05:58:00]

https://chromiumcodereview.appspot.com/11348199/diff/12002/chrome/android/tes...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/chrome/android/tes...
chrome/android/testshell/java/AndroidManifest.xml:70: <uses-permission
android:name="android.permission.READ_PHONE_STATE"/>
Nit: sort

https://chromiumcodereview.appspot.com/11348199/diff/12002/content/browser/an...
File content/browser/android/media_player_manager_android.cc (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/content/browser/an...
content/browser/android/media_player_manager_android.cc:227: Send(new
MediaPlayerMsg_DidMediaPlayerPause(routing_id(), player_id));
Have you tested fullscreen? FullscreenPlayerPause also calls Pause on the
player.

(not familiar enough with the relevant APIs but lots of fullscreen-specific code
so I want to make sure)

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:20:
class MediaPlayerListener extends PhoneStateListener implements
MediaPlayer.OnPreparedListener,
What about composing a PhoneStateListener. Then each class is only covering one
aspect.

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
File media/base/android/media_player_listener.cc (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
media/base/android/media_player_listener.cc:47: if (env &&
!j_media_player_listener_.is_null()) {
Just change the DCHECK above to CHECK and remove the "env" check

[Miguel Garcia, 2012-11-27 17:24:06]

https://chromiumcodereview.appspot.com/11348199/diff/12002/chrome/android/tes...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/chrome/android/tes...
chrome/android/testshell/java/AndroidManifest.xml:70: <uses-permission
android:name="android.permission.READ_PHONE_STATE"/>
On 2012/11/27 05:58:00, Yaron wrote:
> Nit: sort

Done.

https://chromiumcodereview.appspot.com/11348199/diff/12002/content/browser/an...
File content/browser/android/media_player_manager_android.cc (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/content/browser/an...
content/browser/android/media_player_manager_android.cc:227: Send(new
MediaPlayerMsg_DidMediaPlayerPause(routing_id(), player_id));
Yes, played something in full screen mode and called myself. The video paused
nice in this case.

On 2012/11/27 05:58:00, Yaron wrote:
> Have you tested fullscreen? FullscreenPlayerPause also calls Pause on the
> player.
> 
> (not familiar enough with the relevant APIs but lots of fullscreen-specific
code
> so I want to make sure)

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:20:
class MediaPlayerListener extends PhoneStateListener implements
MediaPlayer.OnPreparedListener,
I kind of like the idea of having a single class that worries about all the
reasons that will cause pausing the player without user intervention. Before all
listed reasons came from MediaPlayer but unless they grow much more it's nice to
have them all in a single place instead of spread around the code base.

If you feel strongly about it I'll change it of course. I am still a guest here
:)


On 2012/11/27 05:58:00, Yaron wrote:
> What about composing a PhoneStateListener. Then each class is only covering
one
> aspect.

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
File media/base/android/media_player_listener.cc (right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
media/base/android/media_player_listener.cc:47: if (env &&
!j_media_player_listener_.is_null()) {
On 2012/11/27 05:58:00, Yaron wrote:
> Just change the DCHECK above to CHECK and remove the "env" check

Done.

[Yaron, 2012-11-27 18:12:25]

lgtm

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:20:
class MediaPlayerListener extends PhoneStateListener implements
MediaPlayer.OnPreparedListener,
On 2012/11/27 17:24:06, Miguel Garcia wrote:
> I kind of like the idea of having a single class that worries about all the
> reasons that will cause pausing the player without user intervention. Before
all
> listed reasons came from MediaPlayer but unless they grow much more it's nice
to
> have them all in a single place instead of spread around the code base.
> 
> If you feel strongly about it I'll change it of course. I am still a guest
here
> :)
> 
> 
> On 2012/11/27 05:58:00, Yaron wrote:
> > What about composing a PhoneStateListener. Then each class is only covering
> one
> > aspect.
> 

I was thinking an inner class (and thus not very far away) but I don't feel
overly strongly about it.

[Miguel Garcia, 2012-11-27 18:40:01]

Thanks Yaron,

Ami can you have a look? I need owners approval for
webkit/media/android/webmediaplayer_in_process_android.h and
webkit/media/android/webmediaplayer_in_process_android.cc

Yaron do you think we could add a few owners from android in that directory? you
an qinmin perhaps?

On 2012/11/27 18:12:25, Yaron wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
> File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/11348199/diff/12002/media/base/android...
> media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:20:
> class MediaPlayerListener extends PhoneStateListener implements
> MediaPlayer.OnPreparedListener,
> On 2012/11/27 17:24:06, Miguel Garcia wrote:
> > I kind of like the idea of having a single class that worries about all the
> > reasons that will cause pausing the player without user intervention. Before
> all
> > listed reasons came from MediaPlayer but unless they grow much more it's
nice
> to
> > have them all in a single place instead of spread around the code base.
> > 
> > If you feel strongly about it I'll change it of course. I am still a guest
> here
> > :)
> > 
> > 
> > On 2012/11/27 05:58:00, Yaron wrote:
> > > What about composing a PhoneStateListener. Then each class is only
covering
> > one
> > > aspect.
> > 
> 
> I was thinking an inner class (and thus not very far away) but I don't feel
> overly strongly about it.

[Ami GONE FROM CHROMIUM, 2012-11-27 18:43:26]

webkit/media/android OWNERS lgtm.

Please copy media/base/android/OWNERS into w/m/a/OWNERS (in another CL, obvs).

[commit-bot: I haz the power, 2012-11-27 18:51:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/11348199/7004

[Yaron, 2012-11-27 18:55:19]

Min should really be owner there and in media/base/android but he's not
committer yet. Just nominated him.

[Miguel Garcia, 2012-11-27 19:01:16]

On 2012/11/27 18:55:19, Yaron wrote:
> Min should really be owner there and in media/base/android but he's not
> committer yet. Just nominated him.

Sent https://codereview.chromium.org/11414185/ to copy the owners file, once Min
is accepted as a committer we can add him to both.

[commit-bot: I haz the power, 2012-11-28 10:02:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/11348199/7004

[commit-bot: I haz the power, 2012-11-28 14:14:59]

Change committed as 169922