Tear down ContentVideoView from content-layer

Tear down ContentVideoView from content-layer

Background: ContentVideoView displays fullscreen video in a
VideoViewSurface. Currently, each Android content embedder
(WebView, ContentShell, and ChromeShell) is responsible for
releasing this surface when exiting fullscreen.

Problem: Releasing ContentVideoView from each embedder leads to
code duplication. Worse, this was never done for the WebView:
when it exits fullscreen video playback, AwContents's
exitFullScreen removes the VideoViewSurface which triggers
ContentVideoView's surfaceDestroyed(). surfaceDestroyed()
releases the MediaPlayerAndroid. Releasing the MediaPlayerAndroid
during playback gives a frozen video (see bug).

Solution: For all content embedders on Android, tear down
ContentVideoView, from content-layer (before the embedder
specific "exit fullscreen"-work).

BUG=449152
TEST=For unencrypted video, manually verify that video playback
continues inline when exiting fullscreen (tested in
com.android.browser, ContentShell and ChromeShell). Added
automatic test for VIDEO_HOLE-playback in WebView.

Committed: https://crrev.com/61288e6cf8a84fb0f11556231deb29b25e24e180
Cr-Commit-Position: refs/heads/master@{#313531}

[Hugo Holgersson, 2015-01-15 14:59:42]

hugo.holgersson@sonymobile.com changed reviewers:
+ igsolla@chromium.org

[Hugo Holgersson, 2015-01-15 14:59:42]

@igsolla Can you give me some feedback on this attempt?

[Ignacio Solla, 2015-01-16 11:25:00]

Thanks for fixing this!

https://codereview.chromium.org/845193005/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java
(right):

https://codereview.chromium.org/845193005/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:127:
videoView.exitFullscreen(false);
This seems ok, but I think that we will have a redundant call to
ContentVideoView.destroyContentVideoView (one from here and one later from
native). Not sure if destroying the ContentVideoView before native is done with
it will cause any problem, probably not. 

An alternative, not sure if it would work, is to hold a reference to the
videoView from enterFullscreenVideo. Then in here if videoView != null remove it
from the mCustomView. So we are reverting the operations in the same order. In
other words:

1. enterFullsceen is called and creates the mCustomView
2. enterFullscreenVideo is called, which adds the videoView to the mCustomView
3. exitFullscreen is called, which first removes the videoView from mCustomView
and sets it to null,
4. and then sets the mCustomView to null. 

Will that also fix the problem?

[Hugo Holgersson, 2015-01-16 16:13:24]

hugo.holgersson@sonymobile.com changed reviewers:
+ qinmin@chromium.org

[Hugo Holgersson, 2015-01-16 16:13:25]

@qinmin, PTAL.

https://codereview.chromium.org/845193005/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java
(right):

https://codereview.chromium.org/845193005/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:127:
videoView.exitFullscreen(false);
On 2015/01/16 11:25:00, Ignacio Solla wrote:
> This seems ok, but I think that we will have a redundant call to
> ContentVideoView.destroyContentVideoView (one from here and one later from
> native). Not sure if destroying the ContentVideoView before native is done
with
> it will cause any problem, probably not. 
Done. Moved destroyContentVideoView to scope of code that is only executed once.

>
> An alternative, not sure if it would work, is to hold a reference to the
> videoView from enterFullscreenVideo. Then in here if videoView != null remove
it
> from the mCustomView. So we are reverting the operations in the same order. In
> other words:
> 
> 1. enterFullsceen is called and creates the mCustomView
> 2. enterFullscreenVideo is called, which adds the videoView to the mCustomView
> 3. exitFullscreen is called, which first removes the videoView from
mCustomView
> and sets it to null,
> 4. and then sets the mCustomView to null. 
> 
> Will that also fix the problem?
No. mCustomView.removeView(savedReferenceToContentVideoView) also triggers
surfaceDestroyed(). The problem is that surfaceDestroyed() does
exitFullscreen(true), not exitFullscreen(false). 

I believe that tearing down ContentVideoView should be done in a controlled way
(before surfaceDestroyed()). surfaceDestroyed() can do cleanup, but only as a
last resort when a user suddenly quits the app (see qinmin's comment in
content_video_view.h). Only when a user suddenly quits the app the
MediaPlayerAndroid should be released, right?

[Ignacio Solla, 2015-01-16 16:20:35]

lgtm

[qinmin, 2015-01-16 18:08:18]

I don't quite understand the bug. Chrome never release MediaPlayerAndroid upon
surface destruction. 
So is this a webview only bug?

[Hugo Holgersson, 2015-01-16 19:17:24]

Patchset #3 (id:40001) has been deleted

[Hugo Holgersson, 2015-01-16 19:37:51]

On 2015/01/16 18:08:18, qinmin wrote:
> I don't quite understand the bug. Chrome never release MediaPlayerAndroid upon
> surface destruction. 
> So is this a webview only bug?
Yes.

> So we are reverting the operations in the same order.
I realized that when going in and out from fullscreen video playback, the
current order of hits in AwContentViewClient is:
1. enterFullscreen
2. enterFullscreenVideo
3. exitFullscreen
4. exitFullscreenVideo

Even tough exitFullscreenVideo is an no-op at the moment, we actually want this
order, right?
1. enterFullscreen
2. enterFullscreenVideo
3. exitFullscreenVideo
4. exitFullscreen

To get this order we have to tear down ContentVideoView from outside of
AwContentViewClient. This makes more sense because messages should go from
ContentVideoView to its client (not from the client to the ContentVideoView),
right? AwContentViewClient is a ContentVideoViewClient.

In Patch Set 3 I moved the fix from the client to AwWebContentsDelegateAdapter.
This is copied from the previous code written by qinmin* so it should be safer
than Patch Set 2. Tests passes, but PTAL.

*
https://codereview.chromium.org/618013003/diff/350001/android_webview/java/sr...

[Ignacio Solla, 2015-01-19 11:58:17]

https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
ContentVideoView videoView = ContentVideoView.getContentVideoView();
Thinking more about it this seems unfortunate. Why is the content embedder
involved in tearing down the ContentVideoView in a controlled way? When exiting
fullscreen the content layer should tear down the ContentVideoView by itself
without releasing the media player; without involvement from the content
embedder. The responsibility of managing the ContentVideoView should belong to
the content layer, otherwise every single content embedder would need to be
aware of this corner case.

[Ignacio Solla, 2015-01-19 12:04:12]

On 2015/01/19 11:58:17, Ignacio Solla wrote:
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> File
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> (right):
> 
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> ContentVideoView videoView = ContentVideoView.getContentVideoView();
> Thinking more about it this seems unfortunate. Why is the content embedder
> involved in tearing down the ContentVideoView in a controlled way? When
exiting
> fullscreen the content layer should tear down the ContentVideoView by itself
> without releasing the media player; without involvement from the content
> embedder. The responsibility of managing the ContentVideoView should belong to
> the content layer, otherwise every single content embedder would need to be
> aware of this corner case.

In other words, perhaps we should do this before calling
Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab
in
https://code.google.com/p/chromium/codesearch#chromium/src/components/web_con...

[Hugo Holgersson, 2015-01-19 17:10:14]

> > When exiting fullscreen the content layer should tear down the
ContentVideoView by
> > itself without releasing the media player; without involvement from the
content
> > embedder.
Yeah, I totally agree.

Debugging showed me that BrowserMediaPlayerManager::OnExitFullscreen is never
called. That might be the real problem?
BrowserMediaPlayerManager::OnExitFullscreen does what we want, i.e triggers an
exitFullscreen(false) in ContentVideoView.java. I think a proper fix is to run
BrowserMediaPlayerManager::OnExitFullscreen before
WebContentsImpl::ExitFullscreenMode somehow?

[Hugo Holgersson, 2015-01-20 12:15:29]

https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
ContentVideoView videoView = ContentVideoView.getContentVideoView();
On 2015/01/19 11:58:17, Ignacio Solla wrote:
> Thinking more about it this seems unfortunate. Why is the content embedder
> involved in tearing down the ContentVideoView in a controlled way? When
exiting
> fullscreen the content layer should tear down the ContentVideoView by itself
> without releasing the media player; without involvement from the content
> embedder. The responsibility of managing the ContentVideoView should belong to
> the content layer, otherwise every single content embedder would need to be
> aware of this corner case.
Seems like praxis actually is that embedders must take care of their fullscreen
surfaces. WebContents outsources the tear down work to its delegate. Examples:
ChromeShell's delegate:
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
ContentShell's delegate:
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
Chrome on Android's delegate: I suppose the Java-side of Chrome's
WebContentsDelegate is closed source so I cannot confirm this, but at least
Chrome Beta 40.0.2214.87 does not have this problem.

Until we have a generic way to make content tear down ContenVideoView regardless
of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen), to avoid
rewriting the other embedders, we could maybe, for now, let WebView follow this
praxis too?

[qinmin, 2015-01-20 21:45:54]

lgtm

[Ignacio Solla, 2015-01-21 11:17:05]

On 2015/01/20 12:15:29, Hugo Holgersson wrote:
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> File
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> (right):
> 
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> ContentVideoView videoView = ContentVideoView.getContentVideoView();
> On 2015/01/19 11:58:17, Ignacio Solla wrote:
> > Thinking more about it this seems unfortunate. Why is the content embedder
> > involved in tearing down the ContentVideoView in a controlled way? When
> exiting
> > fullscreen the content layer should tear down the ContentVideoView by itself
> > without releasing the media player; without involvement from the content
> > embedder. The responsibility of managing the ContentVideoView should belong
to
> > the content layer, otherwise every single content embedder would need to be
> > aware of this corner case.
> Seems like praxis actually is that embedders must take care of their
fullscreen
> surfaces. WebContents outsources the tear down work to its delegate. Examples:
> ChromeShell's delegate:
>
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
> ContentShell's delegate:
>
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
> Chrome on Android's delegate: I suppose the Java-side of Chrome's
> WebContentsDelegate is closed source so I cannot confirm this, but at least
> Chrome Beta 40.0.2214.87 does not have this problem.

Yes, we're repeating these 3 lines of code all over the place:
if (!mIsFullscreen) {
    ContentVideoView videoView = ContentVideoView.getContentVideoView();
    if (videoView != null) videoView.exitFullscreen(false);
}

I think it'd be better to move them to content/ and only have to maintain 1
copy.

 
> Until we have a generic way to make content tear down ContenVideoView
regardless
> of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen), to
avoid
> rewriting the other embedders, we could maybe, for now, let WebView follow
this
> praxis too?

Is ContentVideoView.surfaceDestroyed called before
FullscreenController::didExitFullScreen()?

a) If it is NOT called before then I think this would be the correct solution:

Add a new ExitFullscreen method to WebMediaPlayer.h and
webmediaplayer_android.[h|cc]. Its implementation would call
RendererMediaPlayerManager::ExitFullscreen. This is similar to EnterFullscreen.

FullscreenController::didExitFullScreen will then call
webMediaPlayer()->exitFullscreen(), similarly to what happens in
FullscreenController::didEnterFullScreen.

b) If ContentVideoView.surfaceDestroyed is called before
FullscreenController::didExitFullScreen() then perhaps we could tear down the
ContentVideoView before calling
Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab in
web_contents_delegate_android.cc. This is not ideal, but it would avoid having
to duplicate the logic all over the place.

[Ignacio Solla, 2015-01-21 11:33:03]

On 2015/01/21 11:17:05, Ignacio Solla wrote:
> On 2015/01/20 12:15:29, Hugo Holgersson wrote:
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > File
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> > ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > On 2015/01/19 11:58:17, Ignacio Solla wrote:
> > > Thinking more about it this seems unfortunate. Why is the content embedder
> > > involved in tearing down the ContentVideoView in a controlled way? When
> > exiting
> > > fullscreen the content layer should tear down the ContentVideoView by
itself
> > > without releasing the media player; without involvement from the content
> > > embedder. The responsibility of managing the ContentVideoView should
belong
> to
> > > the content layer, otherwise every single content embedder would need to
be
> > > aware of this corner case.
> > Seems like praxis actually is that embedders must take care of their
> fullscreen
> > surfaces. WebContents outsources the tear down work to its delegate.
Examples:
> > ChromeShell's delegate:
> >
>
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
> > ContentShell's delegate:
> >
>
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
> > Chrome on Android's delegate: I suppose the Java-side of Chrome's
> > WebContentsDelegate is closed source so I cannot confirm this, but at least
> > Chrome Beta 40.0.2214.87 does not have this problem.
> 
> Yes, we're repeating these 3 lines of code all over the place:
> if (!mIsFullscreen) {
>     ContentVideoView videoView = ContentVideoView.getContentVideoView();
>     if (videoView != null) videoView.exitFullscreen(false);
> }
> 
> I think it'd be better to move them to content/ and only have to maintain 1
> copy.
> 
>  
> > Until we have a generic way to make content tear down ContenVideoView
> regardless
> > of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen), to
> avoid
> > rewriting the other embedders, we could maybe, for now, let WebView follow
> this
> > praxis too?
> 
> Is ContentVideoView.surfaceDestroyed called before
> FullscreenController::didExitFullScreen()?
> 
> a) If it is NOT called before then I think this would be the correct solution:
> 
> Add a new ExitFullscreen method to WebMediaPlayer.h and
> webmediaplayer_android.[h|cc]. Its implementation would call
> RendererMediaPlayerManager::ExitFullscreen. This is similar to
EnterFullscreen.
> 
> FullscreenController::didExitFullScreen will then call
> webMediaPlayer()->exitFullscreen(), similarly to what happens in
> FullscreenController::didEnterFullScreen.
> 
> b) If ContentVideoView.surfaceDestroyed is called before
> FullscreenController::didExitFullScreen() then perhaps we could tear down the
> ContentVideoView before calling
> Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab in
> web_contents_delegate_android.cc. This is not ideal, but it would avoid having
> to duplicate the logic all over the place.

BTW, we were doing a) in the past, see
https://codereview.chromium.org/290643005/. Min is there a reason why the call
to exitFullscreen was removed? I think we should probably need to call that.

Hugo, proposal a) just ensures that BrowserMediaPlayerManager::OnExitFullscreen
will be called, but discuss with Min if that's feasible.

[Hugo Holgersson, 2015-01-21 13:57:25]

On 2015/01/21 11:33:03, Ignacio Solla wrote:
> On 2015/01/21 11:17:05, Ignacio Solla wrote:
> > On 2015/01/20 12:15:29, Hugo Holgersson wrote:
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > > File
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> > > ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > > On 2015/01/19 11:58:17, Ignacio Solla wrote:
> > > > Thinking more about it this seems unfortunate. Why is the content
embedder
> > > > involved in tearing down the ContentVideoView in a controlled way? When
> > > exiting
> > > > fullscreen the content layer should tear down the ContentVideoView by
> itself
> > > > without releasing the media player; without involvement from the content
> > > > embedder. The responsibility of managing the ContentVideoView should
> belong
> > to
> > > > the content layer, otherwise every single content embedder would need to
> be
> > > > aware of this corner case.
> > > Seems like praxis actually is that embedders must take care of their
> > fullscreen
> > > surfaces. WebContents outsources the tear down work to its delegate.
> Examples:
> > > ChromeShell's delegate:
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
> > > ContentShell's delegate:
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
> > > Chrome on Android's delegate: I suppose the Java-side of Chrome's
> > > WebContentsDelegate is closed source so I cannot confirm this, but at
least
> > > Chrome Beta 40.0.2214.87 does not have this problem.
> > 
> > Yes, we're repeating these 3 lines of code all over the place:
> > if (!mIsFullscreen) {
> >     ContentVideoView videoView = ContentVideoView.getContentVideoView();
> >     if (videoView != null) videoView.exitFullscreen(false);
> > }
> > 
> > I think it'd be better to move them to content/ and only have to maintain 1
> > copy.
> > 
> >  
> > > Until we have a generic way to make content tear down ContenVideoView
> > regardless
> > > of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen), to
> > avoid
> > > rewriting the other embedders, we could maybe, for now, let WebView follow
> > this
> > > praxis too?
> > 
> > Is ContentVideoView.surfaceDestroyed called before
> > FullscreenController::didExitFullScreen()?
> > 
> > a) If it is NOT called before then I think this would be the correct
solution:
> > 
> > Add a new ExitFullscreen method to WebMediaPlayer.h and
> > webmediaplayer_android.[h|cc]. Its implementation would call
> > RendererMediaPlayerManager::ExitFullscreen. This is similar to
> EnterFullscreen.
> > 
> > FullscreenController::didExitFullScreen will then call
> > webMediaPlayer()->exitFullscreen(), similarly to what happens in
> > FullscreenController::didEnterFullScreen.
> > 
> > b) If ContentVideoView.surfaceDestroyed is called before
> > FullscreenController::didExitFullScreen() then perhaps we could tear down
the
> > ContentVideoView before calling
> > Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab in
> > web_contents_delegate_android.cc. This is not ideal, but it would avoid
having
> > to duplicate the logic all over the place.
> 
> BTW, we were doing a) in the past, see
> https://codereview.chromium.org/290643005/. Min is there a reason why the call
> to exitFullscreen was removed? I think we should probably need to call that.
> 
> Hugo, proposal a) just ensures that
BrowserMediaPlayerManager::OnExitFullscreen
> will be called, but discuss with Min if that's feasible.

Logs show that ContentVideoView.surfaceDestroyed is called before
FullscreenController::didExitFullScreen().
9329  9329 D HUGO    : 	at
org.chromium.content.browser.ContentVideoView.surfaceDestroyed(ContentVideoView.java:336)
9329  9475 W chromium: [WARNING:FullscreenController.cpp(105)] HUGO 
FullscreenController::didExitFullScreen

I made a first step towards your proposal (b):
https://codereview.chromium.org/865613002

[Hugo Holgersson, 2015-01-23 15:02:52]

On 2015/01/21 13:57:25, Hugo Holgersson wrote:
> On 2015/01/21 11:33:03, Ignacio Solla wrote:
> > On 2015/01/21 11:17:05, Ignacio Solla wrote:
> > > On 2015/01/20 12:15:29, Hugo Holgersson wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > > > File
> > > >
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > > >
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> > > > ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > > > On 2015/01/19 11:58:17, Ignacio Solla wrote:
> > > > > Thinking more about it this seems unfortunate. Why is the content
> embedder
> > > > > involved in tearing down the ContentVideoView in a controlled way?
When
> > > > exiting
> > > > > fullscreen the content layer should tear down the ContentVideoView by
> > itself
> > > > > without releasing the media player; without involvement from the
content
> > > > > embedder. The responsibility of managing the ContentVideoView should
> > belong
> > > to
> > > > > the content layer, otherwise every single content embedder would need
to
> > be
> > > > > aware of this corner case.
> > > > Seems like praxis actually is that embedders must take care of their
> > > fullscreen
> > > > surfaces. WebContents outsources the tear down work to its delegate.
> > Examples:
> > > > ChromeShell's delegate:
> > > >
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
> > > > ContentShell's delegate:
> > > >
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
> > > > Chrome on Android's delegate: I suppose the Java-side of Chrome's
> > > > WebContentsDelegate is closed source so I cannot confirm this, but at
> least
> > > > Chrome Beta 40.0.2214.87 does not have this problem.
> > > 
> > > Yes, we're repeating these 3 lines of code all over the place:
> > > if (!mIsFullscreen) {
> > >     ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > >     if (videoView != null) videoView.exitFullscreen(false);
> > > }
> > > 
> > > I think it'd be better to move them to content/ and only have to maintain
1
> > > copy.
> > > 
> > >  
> > > > Until we have a generic way to make content tear down ContenVideoView
> > > regardless
> > > > of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen),
to
> > > avoid
> > > > rewriting the other embedders, we could maybe, for now, let WebView
follow
> > > this
> > > > praxis too?
> > > 
> > > Is ContentVideoView.surfaceDestroyed called before
> > > FullscreenController::didExitFullScreen()?
> > > 
> > > a) If it is NOT called before then I think this would be the correct
> solution:
> > > 
> > > Add a new ExitFullscreen method to WebMediaPlayer.h and
> > > webmediaplayer_android.[h|cc]. Its implementation would call
> > > RendererMediaPlayerManager::ExitFullscreen. This is similar to
> > EnterFullscreen.
> > > 
> > > FullscreenController::didExitFullScreen will then call
> > > webMediaPlayer()->exitFullscreen(), similarly to what happens in
> > > FullscreenController::didEnterFullScreen.
> > > 
> > > b) If ContentVideoView.surfaceDestroyed is called before
> > > FullscreenController::didExitFullScreen() then perhaps we could tear down
> the
> > > ContentVideoView before calling
> > > Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab in
> > > web_contents_delegate_android.cc. This is not ideal, but it would avoid
> having
> > > to duplicate the logic all over the place.
> > 
> > BTW, we were doing a) in the past, see
> > https://codereview.chromium.org/290643005/. Min is there a reason why the
call
> > to exitFullscreen was removed? I think we should probably need to call that.
> > 
> > Hugo, proposal a) just ensures that
> BrowserMediaPlayerManager::OnExitFullscreen
> > will be called, but discuss with Min if that's feasible.
> 
> Logs show that ContentVideoView.surfaceDestroyed is called before
> FullscreenController::didExitFullScreen().
> 9329  9329 D HUGO    : 	at
>
org.chromium.content.browser.ContentVideoView.surfaceDestroyed(ContentVideoView.java:336)
> 9329  9475 W chromium: [WARNING:FullscreenController.cpp(105)] HUGO 
> FullscreenController::didExitFullScreen
> 
> I made a first step towards your proposal (b):
> https://codereview.chromium.org/865613002

New patch set contains an attempt on proposal b. I also updated CL headline and
description for new solution.

However, I get: ** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  components/web_contents_delegate_android/web_contents_delegate_android.cc
      Illegal include: "content/browser/android/content_video_view.h"
    Because of no rule applying.

Can we relax this? This patch set has that #include commented out for demo
purpose.

FYI. WebContentsDelegateAndroid is not used by ContentShell. ContentShell is a
delegate itself (class Shell : public WebContentsDelegate). So when we move the
"tear down"-code to web_contents_delegate_android.cc we still need a copy in
content_shell/Shell.java.

Ways to avoid the duplicated code in Shell.java? I don't think so :/  We could
let shell.h subclass WebContentsDelegateAndroid instead of
WebContentsDelegate... but I suppose that would conflict with the idea of
keeping the shell minimal. If we really want zero duplication we must do "tear
down" at web_contents_impl.cc's ExitFullscreenMode() before
delegate_->ExitFullscreenModeForTab. But since web_contents_impl.cc is shared
among all platforms, we do not want to pollute it with Android-macros...

[Ignacio Solla, 2015-01-23 16:26:12]

On 2015/01/23 15:02:52, Hugo Holgersson wrote:
> On 2015/01/21 13:57:25, Hugo Holgersson wrote:
> > On 2015/01/21 11:33:03, Ignacio Solla wrote:
> > > On 2015/01/21 11:17:05, Ignacio Solla wrote:
> > > > On 2015/01/20 12:15:29, Hugo Holgersson wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > > > > File
> > > > >
> > > >
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/845193005/diff/60001/android_webview/java/src...
> > > > >
> > > >
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:221:
> > > > > ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > > > > On 2015/01/19 11:58:17, Ignacio Solla wrote:
> > > > > > Thinking more about it this seems unfortunate. Why is the content
> > embedder
> > > > > > involved in tearing down the ContentVideoView in a controlled way?
> When
> > > > > exiting
> > > > > > fullscreen the content layer should tear down the ContentVideoView
by
> > > itself
> > > > > > without releasing the media player; without involvement from the
> content
> > > > > > embedder. The responsibility of managing the ContentVideoView should
> > > belong
> > > > to
> > > > > > the content layer, otherwise every single content embedder would
need
> to
> > > be
> > > > > > aware of this corner case.
> > > > > Seems like praxis actually is that embedders must take care of their
> > > > fullscreen
> > > > > surfaces. WebContents outsources the tear down work to its delegate.
> > > Examples:
> > > > > ChromeShell's delegate:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/chrome/android/shell/java/sr...
> > > > > ContentShell's delegate:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/833363002/diff/1/content/shell/android/java/s...
> > > > > Chrome on Android's delegate: I suppose the Java-side of Chrome's
> > > > > WebContentsDelegate is closed source so I cannot confirm this, but at
> > least
> > > > > Chrome Beta 40.0.2214.87 does not have this problem.
> > > > 
> > > > Yes, we're repeating these 3 lines of code all over the place:
> > > > if (!mIsFullscreen) {
> > > >     ContentVideoView videoView = ContentVideoView.getContentVideoView();
> > > >     if (videoView != null) videoView.exitFullscreen(false);
> > > > }
> > > > 
> > > > I think it'd be better to move them to content/ and only have to
maintain
> 1
> > > > copy.
> > > > 
> > > >  
> > > > > Until we have a generic way to make content tear down ContenVideoView
> > > > regardless
> > > > > of embedder (maybe using BrowserMediaPlayerManager::OnExitFullscreen),
> to
> > > > avoid
> > > > > rewriting the other embedders, we could maybe, for now, let WebView
> follow
> > > > this
> > > > > praxis too?
> > > > 
> > > > Is ContentVideoView.surfaceDestroyed called before
> > > > FullscreenController::didExitFullScreen()?
> > > > 
> > > > a) If it is NOT called before then I think this would be the correct
> > solution:
> > > > 
> > > > Add a new ExitFullscreen method to WebMediaPlayer.h and
> > > > webmediaplayer_android.[h|cc]. Its implementation would call
> > > > RendererMediaPlayerManager::ExitFullscreen. This is similar to
> > > EnterFullscreen.
> > > > 
> > > > FullscreenController::didExitFullScreen will then call
> > > > webMediaPlayer()->exitFullscreen(), similarly to what happens in
> > > > FullscreenController::didEnterFullScreen.
> > > > 
> > > > b) If ContentVideoView.surfaceDestroyed is called before
> > > > FullscreenController::didExitFullScreen() then perhaps we could tear
down
> > the
> > > > ContentVideoView before calling
> > > > Java_WebContentsDelegateAndroid_toggleFullscreenModeForTab in
> > > > web_contents_delegate_android.cc. This is not ideal, but it would avoid
> > having
> > > > to duplicate the logic all over the place.
> > > 
> > > BTW, we were doing a) in the past, see
> > > https://codereview.chromium.org/290643005/. Min is there a reason why the
> call
> > > to exitFullscreen was removed? I think we should probably need to call
that.
> > > 
> > > Hugo, proposal a) just ensures that
> > BrowserMediaPlayerManager::OnExitFullscreen
> > > will be called, but discuss with Min if that's feasible.
> > 
> > Logs show that ContentVideoView.surfaceDestroyed is called before
> > FullscreenController::didExitFullScreen().
> > 9329  9329 D HUGO    : 	at
> >
>
org.chromium.content.browser.ContentVideoView.surfaceDestroyed(ContentVideoView.java:336)
> > 9329  9475 W chromium: [WARNING:FullscreenController.cpp(105)] HUGO 
> > FullscreenController::didExitFullScreen
> > 
> > I made a first step towards your proposal (b):
> > https://codereview.chromium.org/865613002
> 
> New patch set contains an attempt on proposal b. I also updated CL headline
and
> description for new solution.
> 
> However, I get: ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   components/web_contents_delegate_android/web_contents_delegate_android.cc
>       Illegal include: "content/browser/android/content_video_view.h"
>     Because of no rule applying.
> 
> Can we relax this? This patch set has that #include commented out for demo
> purpose.
> 
> FYI. WebContentsDelegateAndroid is not used by ContentShell. ContentShell is a
> delegate itself (class Shell : public WebContentsDelegate). So when we move
the
> "tear down"-code to web_contents_delegate_android.cc we still need a copy in
> content_shell/Shell.java.
> 
> Ways to avoid the duplicated code in Shell.java? I don't think so :/  We could
> let shell.h subclass WebContentsDelegateAndroid instead of
> WebContentsDelegate... but I suppose that would conflict with the idea of
> keeping the shell minimal. If we really want zero duplication we must do "tear
> down" at web_contents_impl.cc's ExitFullscreenMode() before
> delegate_->ExitFullscreenModeForTab. But since web_contents_impl.cc is shared
> among all platforms, we do not want to pollute it with Android-macros...

Sorry I didn't notice this before. Components are only allowed to depend on the
public API of content. So I believe we have these choices:
1. Add an Android ifdef to web_contents_impl.cc as you explained
2. Make content_video_view.h public
3. Uncomponentize web_contents_delegate_android/, ie. move it to content/

I would prefer not to make content_video_view.h public (it seems to me that
ContentVideoView.java should not be public in the first place). Also, there are
probably some benefits to have web_contents_delegate_android/ as a component, so
the only option we have is 1. I prefer this option because the content layer
should really be responsible of tearing down the ContentVideoView and not the
delegates. Also, I'm not sure that we should worry to much about adding an
Android ifdef because web_contents_impl.cc already contains a few of them. Also,
these would fix the problem for the Shell too.

[Ignacio Solla, 2015-01-26 10:02:07]

https://codereview.chromium.org/845193005/diff/100001/content/shell/android/j...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(left):

https://codereview.chromium.org/845193005/diff/100001/content/shell/android/j...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:264: if
(!mIsFullscreen) {
I think that org/chromium/chrome/browser/fullscreen/FullscreenManager.java also
needs to be updated.

[Hugo Holgersson, 2015-01-26 10:28:58]

https://codereview.chromium.org/845193005/diff/100001/content/shell/android/j...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(left):

https://codereview.chromium.org/845193005/diff/100001/content/shell/android/j...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:264: if
(!mIsFullscreen) {
On 2015/01/26 10:02:07, Ignacio Solla wrote:
> I think that org/chromium/chrome/browser/fullscreen/FullscreenManager.java
also
> needs to be updated.

Done.

[Ignacio Solla, 2015-01-26 10:55:57]

https://codereview.chromium.org/845193005/diff/140001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (left):

https://codereview.chromium.org/845193005/diff/140001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:139: if
(WebContentsDelegate* delegate = web_contents_->GetDelegate())
is this intentional? Don't we still need it?

[Hugo Holgersson, 2015-01-26 11:02:33]

https://codereview.chromium.org/845193005/diff/140001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (left):

https://codereview.chromium.org/845193005/diff/140001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:139: if
(WebContentsDelegate* delegate = web_contents_->GetDelegate())
On 2015/01/26 10:55:57, Ignacio Solla wrote:
> is this intentional? Don't we still need it?
It is intentional. WebContentsImpl::ExitFullscreenMode() also does this. I
removed the lines here so ExitFullscreenModeForTab() does not run twice (I think
it makes mores sense that WebContents calls its delegate).

[Ignacio Solla, 2015-01-26 11:10:01]

lgtm

Thanks Hugo for fixing this and making the codebase better!

[Hugo Holgersson, 2015-01-26 11:21:27]

hugo.holgersson@sonymobile.com changed reviewers:
+ mkosiba@chromium.org, tedchoc@chromium.org

[Hugo Holgersson, 2015-01-26 11:23:25]

On 2015/01/26 11:10:01, Ignacio Solla wrote:
> lgtm
> 
> Thanks Hugo for fixing this and making the codebase better!

@mkosiba PTAL at android_webview/
@tedchoc PTAL at content/shell/android/, chrome/android/,
content/public/android/, content/browser/web_contents
@qinmin PTAL at browser_media_player_manager.cc

[mkosiba (inactive), 2015-01-26 12:03:05]

lgtm

https://codereview.chromium.org/845193005/diff/140001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java
(right):

https://codereview.chromium.org/845193005/diff/140001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:486:
// TODO: Test that inline video is actually displayed.
do we have a bug tracking this?

[Hugo Holgersson, 2015-01-26 14:37:36]

https://codereview.chromium.org/845193005/diff/140001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java
(right):

https://codereview.chromium.org/845193005/diff/140001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:486:
// TODO: Test that inline video is actually displayed.
On 2015/01/26 12:03:05, mkosiba wrote:
> do we have a bug tracking this?

Done. I created crbug.com/451995 for this.

[Ted C, 2015-01-26 18:23:14]

On 2015/01/26 11:23:25, Hugo Holgersson wrote:
> On 2015/01/26 11:10:01, Ignacio Solla wrote:
> > lgtm
> > 
> > Thanks Hugo for fixing this and making the codebase better!
> 
> @mkosiba PTAL at android_webview/
> @tedchoc PTAL at content/shell/android/, chrome/android/,
> content/public/android/, content/browser/web_contents

lgtm

> @qinmin PTAL at browser_media_player_manager.cc

[Ted C, 2015-01-26 18:25:11]

On 2015/01/26 18:23:14, Ted C wrote:
> On 2015/01/26 11:23:25, Hugo Holgersson wrote:
> > On 2015/01/26 11:10:01, Ignacio Solla wrote:
> > > lgtm
> > > 
> > > Thanks Hugo for fixing this and making the codebase better!
> > 
> > @mkosiba PTAL at android_webview/
> > @tedchoc PTAL at content/shell/android/, chrome/android/,
> > content/public/android/, content/browser/web_contents
> 
> lgtm
> 
> > @qinmin PTAL at browser_media_player_manager.cc

Also, you'll need another owner for web_contents_impl.cc as I can only
review the *android* files in that directory.

[Hugo Holgersson, 2015-01-27 10:20:02]

hugo.holgersson@sonymobile.com changed reviewers:
+ sievers@chromium.org

[Hugo Holgersson, 2015-01-27 10:22:40]

On 2015/01/26 18:25:11, Ted C wrote:
> On 2015/01/26 18:23:14, Ted C wrote:
> > On 2015/01/26 11:23:25, Hugo Holgersson wrote:
> > > On 2015/01/26 11:10:01, Ignacio Solla wrote:
> > > > lgtm
> > > > 
> > > > Thanks Hugo for fixing this and making the codebase better!
> > > 
> > > @mkosiba PTAL at android_webview/
> > > @tedchoc PTAL at content/shell/android/, chrome/android/,
> > > content/public/android/, content/browser/web_contents
> > 
> > lgtm
> > 
> > > @qinmin PTAL at browser_media_player_manager.cc
> 
> Also, you'll need another owner for web_contents_impl.cc as I can only
> review the *android* files in that directory.

@sievers PTAL at content/browser/web_contents/web_contents_impl.cc

[Ignacio Solla, 2015-01-27 13:46:16]

Torne: for android_webview's OWNER rubberstamp

[Ignacio Solla, 2015-01-27 13:47:36]

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

[Torne, 2015-01-27 14:18:18]

lgtm

[no sievers, 2015-01-27 19:35:08]

lgtm

[Hugo Holgersson, 2015-01-28 15:26:06]

The CQ bit was checked by hugo.holgersson@sonymobile.com

[commit-bot: I haz the power, 2015-01-28 15:26:23]

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

[commit-bot: I haz the power, 2015-01-28 15:35:03]

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

[commit-bot: I haz the power, 2015-01-28 15:35:04]

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

[Hugo Holgersson, 2015-01-28 15:55:04]

The CQ bit was checked by hugo.holgersson@sonymobile.com

[commit-bot: I haz the power, 2015-01-28 15:55:40]

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

[commit-bot: I haz the power, 2015-01-28 17:05:17]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-01-28 17:05:55]

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