[Android] Decouple pausing video from RenderView pause.

[Android] Decouple pausing video from RenderView pause.
    
This way we can trigger video to pause from more than one place.

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

[benm (inactive), 2013-09-18 18:18:41]

ptal

[benm (inactive), 2013-09-18 18:26:51]

Bo says this is a Bad Idea. Will look at how we can plumb this in to just pause
the video.

[benm (inactive), 2013-09-19 01:44:30]

OK, latest PS should not be a bad idea! PTAL.

[boliu, 2013-09-19 01:58:39]

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:689:
ContentViewCore::FromWebContents(web_contents_.get())->PauseVideo();
condition on if (paused)

https://codereview.chromium.org/23464080/diff/7001/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/23464080/diff/7001/content/common/view_messag...
content/common/view_messages.h:2283: 
nit: remove new lines

https://codereview.chromium.org/23464080/diff/7001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5754: void RenderViewImpl::OnPauseVideo() {
Method is no-op on desktop platforms, which is extremely weird. Maybe we should
ifdef everything, including the ipc..?

Or just add a comment.

[joth, 2013-09-19 02:41:50]

lgtm w/ bo's nits too

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:689:
ContentViewCore::FromWebContents(web_contents_.get())->PauseVideo();
null check it.

https://codereview.chromium.org/23464080/diff/7001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23464080/diff/7001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:354: 
\nn

[benm (inactive), 2013-09-19 03:27:32]

thanks!

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/23464080/diff/7001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:689:
ContentViewCore::FromWebContents(web_contents_.get())->PauseVideo();
Done both, thanks!

https://codereview.chromium.org/23464080/diff/7001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23464080/diff/7001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:354: 
On 2013/09/19 02:41:50, joth wrote:
> \nn

Done.

https://codereview.chromium.org/23464080/diff/7001/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/23464080/diff/7001/content/common/view_messag...
content/common/view_messages.h:2283: 
On 2013/09/19 01:58:39, boliu wrote:
> nit: remove new lines

Done.

https://codereview.chromium.org/23464080/diff/7001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5754: void RenderViewImpl::OnPauseVideo() {
On 2013/09/19 01:58:39, boliu wrote:
> Method is no-op on desktop platforms, which is extremely weird. Maybe we
should
> ifdef everything, including the ipc..?
> 
> Or just add a comment.

Done.

[joth, 2013-09-19 03:30:48]

lgtm

[benm (inactive), 2013-09-19 03:31:00]

+palmer for IPC review
+jamesr for content/renderer

[benm (inactive), 2013-09-19 03:35:35]

+bulach for content/public/browser/android

Thanks all!

[bulach, 2013-09-19 09:45:53]

lgtm for android, adding qinmin FYI for video-related changes..

[qinmin, 2013-09-19 16:37:37]

lgtm

https://codereview.chromium.org/23464080/diff/19001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/19001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1933:
media_player_manager_->ReleaseMediaResources();
nit: change the function name of ReleaseMediaResources() to
ReleaseVideoResources() in RenderMediaPlayerManager to be consistent

[benm (inactive), 2013-09-19 17:51:25]

thanks Min!

https://codereview.chromium.org/23464080/diff/19001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/19001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1933:
media_player_manager_->ReleaseMediaResources();
On 2013/09/19 16:37:38, qinmin wrote:
> nit: change the function name of ReleaseMediaResources() to
> ReleaseVideoResources() in RenderMediaPlayerManager to be consistent

Done.

[jamesr, 2013-09-19 21:55:55]

why is pausing a video android-specific?

[benm (inactive), 2013-09-19 22:06:02]

On 2013/09/19 21:55:55, jamesr wrote:
> why is pausing a video android-specific?

Thanks James!

It's the implementation that's android specific - in PS2 I had only the
RenderViewImpl::PauseVideo() function body #ifdef'd, but was advised on the
review to make the entire flow Android specific too, otherwise we'd end up with
an empty function on all platforms other than android.

[palmer, 2013-09-19 22:11:27]

IPC security LGTM.

But can we make more of it get built only on Android (more #ifdefs, and/or
create a content/common/view_messages_android.h, or something)? (Is that even a
good idea?)

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1932: // via WebMediaPlayerAndroid::Destroy
Nit: End sentence with a period.

[jamesr, 2013-09-19 22:12:30]

I don't understand why the implementation is android-specific either.  This
seems like a generic capability (the ability to pause all videos in a view). 
Why isn't this plumbed through public content API?  I can't think of a good
reason for this to be content::ContentViewCore instead of being on WebContents
(or something similar).

[jamesr, 2013-09-19 22:14:17]

+jam.  This is a completely generic capability, but only android has a need for
it right now.  This patch pipes it through android-specific interfaces and
guards the implementation in OS_ANDROID, but that seems very strange.  What do
you think is the right thing to do?

[benm (inactive), 2013-09-19 22:18:39]

On 2013/09/19 22:12:30, jamesr wrote:
> I don't understand why the implementation is android-specific either.  This
> seems like a generic capability (the ability to pause all videos in a view). 
> Why isn't this plumbed through public content API?  I can't think of a good
> reason for this to be content::ContentViewCore instead of being on WebContents
> (or something similar).

Currently we pause video when we get the RenderViewImpl::OnWasHidden IPC and the
code to do it is inside an android ifdef as the media_player_manager_ is an
Android specific member of RenderViewImpl.

It's desirable for us to be able to pause the video in situations other than
RenderViewImpl::WasHidden, so my intent was to just decouple the two things i.e.
OnWasHidden and pausing a video.

[jamesr, 2013-09-19 22:22:32]

On 2013/09/19 22:18:39, benm wrote:
> It's desirable for us to be able to pause the video in situations other than
> RenderViewImpl::WasHidden, so my intent was to just decouple the two things
i.e.
> OnWasHidden and pausing a video.

I get that and agree decoupling is nice.  The question is why is the ability to
pause the videos on a RenderView android-specific?

[benm (inactive), 2013-09-19 22:28:20]

On 2013/09/19 22:22:32, jamesr wrote:
> I get that and agree decoupling is nice.  The question is why is the ability
to
> pause the videos on a RenderView android-specific?

I guess it doesn't have to be :) But I don't know how the other platforms pause
video... I'd be happy to make the function and IPC cross platform and have an
ifdef inside it for Android and a NOTIMPLEMENTED for the other platforms ... is
that along the lines you are thinking?

[qinmin, 2013-09-19 22:46:34]

On 2013/09/19 22:28:20, benm wrote:
> On 2013/09/19 22:22:32, jamesr wrote:
> > I get that and agree decoupling is nice.  The question is why is the ability
> to
> > pause the videos on a RenderView android-specific?
> 
> I guess it doesn't have to be :) But I don't know how the other platforms
pause
> video... I'd be happy to make the function and IPC cross platform and have an
> ifdef inside it for Android and a NOTIMPLEMENTED for the other platforms ...
is
> that along the lines you are thinking?

Currently only android keeps track of all the players inside renderView.
For desktop, there is no such need to pause the video while navigating to
another tab.
So if we make the call non-android specific, i think a NOTIMPLEMENTED makes
sense for the rest platforms, at least for now.

[jamesr, 2013-09-19 23:17:28]

content/renderer/ lgtm

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1929: // unless some audio is playing.
this comment isn't accurate. we look at videos one at a time, so we'll actually
release resources for all videos that do not have audio playing. this comment
implies that if any audio is playing we won't release anything. please fix

[benm (inactive), 2013-09-20 01:25:06]

Thanks folks :)

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1929: // unless some audio is playing.
On 2013/09/19 23:17:28, jamesr wrote:
> this comment isn't accurate. we look at videos one at a time, so we'll
actually
> release resources for all videos that do not have audio playing. this comment
> implies that if any audio is playing we won't release anything. please fix

Done.

https://codereview.chromium.org/23464080/diff/26001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1932: // via WebMediaPlayerAndroid::Destroy
On 2013/09/19 22:11:27, Chromium Palmer wrote:
> Nit: End sentence with a period.

Done.

[commit-bot: I haz the power, 2013-09-20 01:38:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/23464080/40001

[commit-bot: I haz the power, 2013-09-20 05:52:55]

Change committed as 224299