[Android] Stop all media on undoable close.

[Android] Stop all media on undoable close.

-Stop all media playback on an undoable tab close

BUG=388600
Committed: https://crrev.com/8cda4271a88ecf178e1b6104bcca6cb35527f765
Cr-Commit-Position: refs/heads/master@{#292959}

[David Trainor- moved to gerrit, 2014-08-21 23:13:05]

ptal:

qinmin@: general review and content/browser/media stuff
tedchoc@ content/browser/web_contents, content/public, and chrome/android

thanks!

[Ted C, 2014-08-22 01:08:08]

On 2014/08/21 23:13:05, David Trainor wrote:
> ptal:
> 
> qinmin@: general review and content/browser/media stuff
> tedchoc@ content/browser/web_contents, content/public, and chrome/android
> 
> thanks!

Do we stop media when switching tabs?  I was thinking that we could simply
mark a tab as hidden to get this behavior, but I suspect I'm sorely
mistaken.

[David Trainor- moved to gerrit, 2014-08-22 02:33:01]

On 2014/08/22 01:08:08, Ted C wrote:
> On 2014/08/21 23:13:05, David Trainor wrote:
> > ptal:
> > 
> > qinmin@: general review and content/browser/media stuff
> > tedchoc@ content/browser/web_contents, content/public, and chrome/android
> > 
> > thanks!
> 
> Do we stop media when switching tabs?  I was thinking that we could simply
> mark a tab as hidden to get this behavior, but I suspect I'm sorely
> mistaken.

We do mark the tab hidden AFAIK, but that doesn't stop music from playing (I
believe videos do stop).  Music still plays by design.  However in this case
since we're making it seem like the tab actually closed I wanted to stop music
playback as well.

[qinmin, 2014-08-22 21:49:33]

https://codereview.chromium.org/493253002/diff/1/content/browser/media/media_...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/493253002/diff/1/content/browser/media/media_...
content/browser/media/media_web_contents_observer.h:61: void
ReleaseAllMediaPlayers(RenderFrameHost* render_frame_host);
I think you don't need this call. you can just call
MediaWebContentsObserver::GetMediaPlayerManager() in WebContentsAndroid and then
call BrowserMediaPlayerManager::ReleaseAllMediaPlayers();

https://codereview.chromium.org/493253002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/493253002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_android.cc:189:
web_contents_->GetRenderViewHost());
nit: indent -2

https://codereview.chromium.org/493253002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_android.cc:190: if (rvhi &&
rvhi->GetMainFrame()) {
indent -2 and below

[David Trainor- moved to gerrit, 2014-08-25 18:11:43]

https://chromiumcodereview.appspot.com/493253002/diff/1/content/browser/media...
File content/browser/media/media_web_contents_observer.h (right):

https://chromiumcodereview.appspot.com/493253002/diff/1/content/browser/media...
content/browser/media/media_web_contents_observer.h:61: void
ReleaseAllMediaPlayers(RenderFrameHost* render_frame_host);
On 2014/08/22 21:49:32, qinmin wrote:
> I think you don't need this call. you can just call
> MediaWebContentsObserver::GetMediaPlayerManager() in WebContentsAndroid and
then
> call BrowserMediaPlayerManager::ReleaseAllMediaPlayers();

Done.

https://chromiumcodereview.appspot.com/493253002/diff/1/content/browser/web_c...
File content/browser/web_contents/web_contents_android.cc (right):

https://chromiumcodereview.appspot.com/493253002/diff/1/content/browser/web_c...
content/browser/web_contents/web_contents_android.cc:189:
web_contents_->GetRenderViewHost());
On 2014/08/22 21:49:32, qinmin wrote:
> nit: indent -2

Done.

https://chromiumcodereview.appspot.com/493253002/diff/1/content/browser/web_c...
content/browser/web_contents/web_contents_android.cc:190: if (rvhi &&
rvhi->GetMainFrame()) {
On 2014/08/22 21:49:32, qinmin wrote:
> indent -2 and below

Done.

[Ted C, 2014-08-25 18:38:56]

lgtm

https://chromiumcodereview.appspot.com/493253002/diff/20001/content/browser/m...
File content/browser/media/android/browser_media_player_manager.h (right):

https://chromiumcodereview.appspot.com/493253002/diff/20001/content/browser/m...
content/browser/media/android/browser_media_player_manager.h:73: void
ReleaseAllMediaPlayers();
I would add a comment for what this does.

[Ted C, 2014-08-25 18:38:57]

lgtm

lgtm

[David Trainor- moved to gerrit, 2014-08-25 18:44:01]

https://chromiumcodereview.appspot.com/493253002/diff/20001/content/browser/m...
File content/browser/media/android/browser_media_player_manager.h (right):

https://chromiumcodereview.appspot.com/493253002/diff/20001/content/browser/m...
content/browser/media/android/browser_media_player_manager.h:73: void
ReleaseAllMediaPlayers();
On 2014/08/25 18:38:56, Ted C wrote:
> I would add a comment for what this does.

Done.

[qinmin, 2014-08-25 18:57:12]

lgtm % nits

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
File content/browser/web_contents/web_contents_android.cc (right):

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_android.cc:190:
web_contents_->GetRenderViewHost());
nit: -2 indent

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_android.cc:192: if (rvhi &&
rvhi->GetMainFrame()) {
nit: nested if statements. Consider rewriting:

if (!rvhi || !rvhi->GetMainFrame())
  return;

.......

[David Trainor- moved to gerrit, 2014-08-25 23:42:15]

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
File content/browser/web_contents/web_contents_android.cc (right):

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_android.cc:190:
web_contents_->GetRenderViewHost());
On 2014/08/25 18:57:12, qinmin wrote:
> nit: -2 indent

Done.

https://chromiumcodereview.appspot.com/493253002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_android.cc:192: if (rvhi &&
rvhi->GetMainFrame()) {
On 2014/08/25 18:57:12, qinmin wrote:
> nit: nested if statements. Consider rewriting:
> 
> if (!rvhi || !rvhi->GetMainFrame())
>   return;
> 
> .......
> 

Done.

[David Trainor- moved to gerrit, 2014-08-25 23:46:32]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-25 23:48:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/60001

[commit-bot: I haz the power, 2014-08-26 02:43:32]

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

[commit-bot: I haz the power, 2014-08-26 02:43:34]

Failed to commit the patch.

[David Trainor- moved to gerrit, 2014-08-26 19:04:59]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-26 19:05:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/60001

[commit-bot: I haz the power, 2014-08-26 19:07:23]

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

[commit-bot: I haz the power, 2014-08-26 19:07:24]

Failed to commit the patch.

[David Trainor- moved to gerrit, 2014-08-27 21:40:19]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-27 21:42:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/80001

[commit-bot: I haz the power, 2014-08-27 22:43:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-27 23:11:35]

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

[commit-bot: I haz the power, 2014-08-27 23:11:37]

Failed to commit the patch.

[David Trainor- moved to gerrit, 2014-08-27 23:28:39]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-27 23:29:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/80001

[commit-bot: I haz the power, 2014-08-27 23:30:53]

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

[commit-bot: I haz the power, 2014-08-27 23:30:54]

Failed to commit the patch.

[David Trainor- moved to gerrit, 2014-08-28 18:20:14]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-08-28 18:21:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/100001

[commit-bot: I haz the power, 2014-08-28 19:12:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-28 20:09:25]

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

[commit-bot: I haz the power, 2014-08-28 20:09:26]

Failed to commit the patch.

[David Trainor- moved to gerrit, 2014-09-02 18:11:43]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-09-02 18:14:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/493253002/100001

[commit-bot: I haz the power, 2014-09-02 18:20:04]

Committed patchset #6 (id:100001) as c4c1b9a06a713ff6db84b060a3190bbf0560290c

[commit-bot: I haz the power, 2014-09-10 03:19:29]

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