Upstream fullscreen video implementation for android

Upstream fullscreen video implementation for android


BUG=136677
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160431

[qinmin, 2012-09-27 16:39:18]

jam: please take a look at view_messages.h (need OWNERS approval)

scherkus: please take a look at the media related changes

[jam, 2012-09-27 17:06:25]

can you move all the media player messages to a separate file, i.e.
content\common\android\media_player_messsages.h? This will make things clearer
(not everything should be in view_messages, that's mostly for RenderView related
stuff) and it'll let you not need top level owners rubberstamps

[qinmin, 2012-09-27 19:57:38]

Done. Added media/media_player_messages.h.

On 2012/09/27 17:06:25, John Abd-El-Malek wrote:
> can you move all the media player messages to a separate file, i.e.
> content\common\android\media_player_messsages.h? This will make things clearer
> (not everything should be in view_messages, that's mostly for RenderView
related
> stuff) and it'll let you not need top level owners rubberstamps

[jam, 2012-09-27 20:26:29]

content outside of android directories lgtm, thanks

[Yaron, 2012-10-02 00:29:23]

content/browser/android looks reasonable although I'm not really familiar with
it. Probably best for scherkus to cover

http://codereview.chromium.org/10979047/diff/4001/content/browser/android/con...
File content/browser/android/content_video_view.cc (right):

http://codereview.chromium.org/10979047/diff/4001/content/browser/android/con...
content/browser/android/content_video_view.cc:132:
j_content_video_view_.Reset();
WHy does exiting full screen just null out the jobject but not destroy the
ContentVideoView?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:145: if
(fullscreen_player_id_ != -1 && fullscreen_player_id_ != player_id)
Do these cases happen? It seems like you shouldn't get an OnEnterFullscreen if
you're already in fullscreen. Could these be DCHECKs instead?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:153: if
(fullscreen_player_id_ == player_id) {
Same here. Is it race-y such that we can't be confident we don't get the wrong
message?

[scherkus (not reviewing), 2012-10-02 16:20:15]

sorry for not getting to this earlier!

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
File content/browser/android/content_video_view.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:93: return player ?
player->GetVideoWidth() : 0;
what are the conditions for player being null?

considering this class is owned + bound to lifetime of MediaPlayerManagerAndroid
it seems we should be able to make stronger assertions on when the player object
is non-null

for example, it it possible to create the CVV inside MPMA::OnEnterFullscreen()
and delete it in OnExitFullscreen()?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:131: if
(!j_content_video_view_.is_null())
when would this be false?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:143: if (player)
nit: if statements >1 line need braces OR negate condition and do an early
return -- your pick :)

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
File content/browser/android/content_video_view.h (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.h:48: void ExitFullscreen(JNIEnv*,
jobject, jboolean release_media_player);
is it guaranteed that no more methods get called on this object after
ExitFullscreen is called?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.h:56: void SetSurface(JNIEnv*,
nit: this looks like it can now fit on one line

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:123:
GetPlayer(player_id)->GetDuration()));
fix indent

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/sur...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/sur...
content/browser/android/surface_texture_peer_browser_impl.cc:43:
player->SetVideoSurface(j_surface);
nit: if statements > 1 line need {}

http://codereview.chromium.org/10979047/diff/7003/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/renderer/media/webm...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:147: if (player)
OOC when would these be false?

[qinmin, 2012-10-03 02:32:47]

http://codereview.chromium.org/10979047/diff/4001/content/browser/android/con...
File content/browser/android/content_video_view.cc (right):

http://codereview.chromium.org/10979047/diff/4001/content/browser/android/con...
content/browser/android/content_video_view.cc:132:
j_content_video_view_.Reset();
The ContentVideoView is destroying itself when this getting called. So we just
nullify the java reference here. The native object is owned by
MediaPlayerManagerAndroid, so we don't need to do anything.
On 2012/10/02 00:29:23, Yaron wrote:
> WHy does exiting full screen just null out the jobject but not destroy the
> ContentVideoView?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
File content/browser/android/content_video_view.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:93: return player ?
player->GetVideoWidth() : 0;
This could happen if a media element switches sources. for example, when youtube
is playing an ads before playing the real video.
In this case, when the first video calls enterfullscreen(), it will never call
exitfullscreen(). So the video view will always on top. However, it will call
destroyPlayer() to remove itself from the MPMA when WebMediaPlayerAndroid dtor
get called.  The video view is not aware of this, so it could still issue calls
to the player.
When the 2nd video came, since they are from the same element, it will call
enterfullscreen to get the video view. And then the video view belongs to the
2nd player.
On 2012/10/02 16:20:16, scherkus wrote:
> what are the conditions for player being null?
> 
> considering this class is owned + bound to lifetime of
MediaPlayerManagerAndroid
> it seems we should be able to make stronger assertions on when the player
object
> is non-null
> 
> for example, it it possible to create the CVV inside MPMA::OnEnterFullscreen()
> and delete it in OnExitFullscreen()?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:131: if
(!j_content_video_view_.is_null())
Removed the if statement since after this statement, j_content_video_view_ is
always null.
When DestroyContentVideoView is called by MediaPlayerManagerAndroid, we ask the
jobject to remove it from the view system. When the view is destroyed, a
SurfaceView.surfaceDestroyed() will get called. This call will call into this
function. DestroyContentVideoView() may already set the j_content_video_view_ to
null.
On 2012/10/02 16:20:16, scherkus wrote:
> when would this be false?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.cc:143: if (player)
On 2012/10/02 16:20:16, scherkus wrote:
> nit: if statements >1 line need braces OR negate condition and do an early
> return -- your pick :)

Done.

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
File content/browser/android/content_video_view.h (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.h:48: void ExitFullscreen(JNIEnv*,
jobject, jboolean release_media_player);
Yes, ExitFullscreen should be the last call the java object can make to the
native object.

On 2012/10/02 16:20:16, scherkus wrote:
> is it guaranteed that no more methods get called on this object after
> ExitFullscreen is called?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/con...
content/browser/android/content_video_view.h:56: void SetSurface(JNIEnv*,
On 2012/10/02 16:20:16, scherkus wrote:
> nit: this looks like it can now fit on one line

Done.

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:123:
GetPlayer(player_id)->GetDuration()));
On 2012/10/02 16:20:16, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:145: if
(fullscreen_player_id_ != -1 && fullscreen_player_id_ != player_id)
Changed to DCHECK
On 2012/10/02 00:29:23, Yaron wrote:
> Do these cases happen? It seems like you shouldn't get an OnEnterFullscreen if
> you're already in fullscreen. Could these be DCHECKs instead?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/med...
content/browser/android/media_player_manager_android.cc:153: if
(fullscreen_player_id_ == player_id) {
This could happen if user hit the back button during fullscreen video play, and
at the same time video.exitfullscreen() gets called.
In this case, when the ipc arrives, fullscreen_player_id is already set to -1.

On 2012/10/02 00:29:23, Yaron wrote:
> Same here. Is it race-y such that we can't be confident we don't get the wrong
> message?

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/sur...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/browser/android/sur...
content/browser/android/surface_texture_peer_browser_impl.cc:43:
player->SetVideoSurface(j_surface);
On 2012/10/02 16:20:16, scherkus wrote:
> nit: if statements > 1 line need {}

Done.

http://codereview.chromium.org/10979047/diff/7003/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

http://codereview.chromium.org/10979047/diff/7003/content/renderer/media/webm...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:147: if (player)
This could happen when a WebMediaPlayer calls enterfullscreen(), and then
immediately deletes it self.
When enterfullscreen is called, an IPC is sent to the browser and
OnDidEnterFullscreen() will be called when browser finished creating the
fullscreen view. However, WebMediaPlayerManager may have already deleted the
player during the mean time.

On 2012/10/02 16:20:16, scherkus wrote:
> OOC when would these be false?

[Yaron, 2012-10-03 23:15:37]

lgtm

[scherkus (not reviewing), 2012-10-04 21:13:41]

lgtm w/ nits

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:145:
DCHECK(fullscreen_player_id_ == -1)
nit: DCHECK_EQ

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:7: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
nit: do you actually need this?

it appears you're only doing pointer comparisons

[scherkus (not reviewing), 2012-10-04 21:13:41]

lgtm w/ nits

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:145:
DCHECK(fullscreen_player_id_ == -1)
nit: DCHECK_EQ

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:7: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
nit: do you actually need this?

it appears you're only doing pointer comparisons

[qinmin, 2012-10-04 23:49:32]

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:145:
DCHECK(fullscreen_player_id_ == -1)
On 2012/10/04 21:13:41, scherkus wrote:
> nit: DCHECK_EQ

Done.

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_manager_android.cc (right):

http://codereview.chromium.org/10979047/diff/22001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_manager_android.cc:7: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
On 2012/10/04 21:13:41, scherkus wrote:
> nit: do you actually need this?
> 
> it appears you're only doing pointer comparisons

Done.

[commit-bot: I haz the power, 2012-10-05 01:05:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/10979047/34001

[commit-bot: I haz the power, 2012-10-05 01:14:18]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 16:39:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/10979047/42001

[commit-bot: I haz the power, 2012-10-05 18:44:23]

Change committed as 160431