Support fullscreen for non-video elements in the WebView.

Support fullscreen for non-video elements in the WebView.

When the browser receives the toggleFullscreenModeForTab
IPC we ask the app to show the custom ViewGroup (containing
a FullscreenView with the web contents in fullscreen). If
later (for the video element case) a ContentVideoView is
created then we add it to the custom ViewGroup.

The next steps (in subsequent changes) are:
1. Add tests for fullscreen for non-media elements in the
   WebView.
2. Changes to ensure that the power saver blocker works
   for video elements contained inside other elements.
3. Changes to ensure that scrolling works in fullscreen.

This change also removes the now unused
disallow_fullscreen_for_non_media_elements setting and
simplifies the JNI interface in ContentVideoView.

BUG=398485
Committed: https://crrev.com/3e28946a9edc2d794a894a6a4a0be965f8b62761
Cr-Commit-Position: refs/heads/master@{#299889}

[Ignacio Solla, 2014-10-02 12:14:08]

igsolla@chromium.org changed reviewers:
+ jam@chromium.org, mkosiba@chromium.org, qinmin@chromium.org,
tsepez@chromium.org

[Ignacio Solla, 2014-10-02 12:14:08]

tsepez: Please review changes in view_messages.h and
common_param_traits_macros.h

jam: Please review changes in content/

mkosiba: Please review changes in android_webview/

qinmin: Please review video changes

[mkosiba (inactive), 2014-10-02 14:19:57]

lgtm

[Tom Sepez, 2014-10-02 17:28:59]

https://codereview.chromium.org/618013003/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/618013003/diff/1/content/common/view_messages...
content/common/view_messages.h:1599: // bound into / out of fullscreen. In the
case of video only at this point
I don't understand the sentence "In the case of video ..."

[Tom Sepez, 2014-10-02 17:31:17]

Messages LGTM otherwise.

[Ignacio Solla, 2014-10-03 09:45:20]

https://codereview.chromium.org/618013003/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/618013003/diff/1/content/common/view_messages...
content/common/view_messages.h:1599: // bound into / out of fullscreen. In the
case of video only at this point
On 2014/10/02 17:28:59, Tom Sepez wrote:
> I don't understand the sentence "In the case of video ..." 

Removed the second sentence, as it is just too verbose.

[qinmin, 2014-10-06 16:55:28]

https://codereview.chromium.org/618013003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2802:
getContentVideoViewClient().enterFullscreenVideo(mVideoView);
we call WebMediaPlayer::enterfullscreen() in
FullscreenController::didEnterFullScreen(), that call will create the
mVideoview. So this call has to be called after that call, which can be easily
broken if someone refactor the code.
I think we need to either remove the WebMediaPlayer::enterfullscreen() in
FullscreenController or add a comments in render_widget.cc to indicate the
importance of the ordering.

[Ignacio Solla, 2014-10-07 18:14:43]

https://codereview.chromium.org/618013003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2802:
getContentVideoViewClient().enterFullscreenVideo(mVideoView);
On 2014/10/06 16:55:28, qinmin wrote:
> we call WebMediaPlayer::enterfullscreen() in
> FullscreenController::didEnterFullScreen(), that call will create the
> mVideoview. So this call has to be called after that call, which can be easily
> broken if someone refactor the code.
> I think we need to either remove the WebMediaPlayer::enterfullscreen() in
> FullscreenController or add a comments in render_widget.cc to indicate the
> importance of the ordering.

I added a comment to render_widget.cc.

As discussed offline, the alternative would be to return a boolean from
FullscreenController.didEnterFullscreen that tells whether the element is a
video, but that would be overkill.

[qinmin, 2014-10-07 22:11:53]

lgtm

[jam, 2014-10-08 22:28:46]

jam@chromium.org changed reviewers:
+ sievers@chromium.org

[jam, 2014-10-08 22:28:46]

rerouting to sievers as I'm overloaded, thanks

[no sievers, 2014-10-10 02:06:15]

Why do we need a fullscreen video view when you go into fullscreen for a
non-video element?
I thought that normally works through compositing with the UI being hidden.

[no sievers, 2014-10-10 02:15:25]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;
Can you avoid holding this ref here?

[Ignacio Solla, 2014-10-10 09:11:20]

On 2014/10/10 02:15:25, sievers wrote:
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
> private ContentVideoView mVideoView;
> Can you avoid holding this ref here?

It turns that we can. There can only be at most 1 single ContentVidewView at any
one time, and ContentVideoView.getContentVideoView returns that instance.

[Ignacio Solla, 2014-10-10 09:21:20]

On 2014/10/10 02:06:15, sievers wrote:
> Why do we need a fullscreen video view when you go into fullscreen for a
> non-video element?
> I thought that normally works through compositing with the UI being hidden.

In short, because we want to avoid having two different Android APIs for
fullscreen, one for video and one for non-video.

By creating a fullscreen view apps that support fullscreen video will start
supporting fullscreen for non-video transparently without having to perform any
changes to their app, as WebChromeClient.{onShow|onHide}CustomView will already
work for non-video.

From the point of view of the app it is probably also simpler to attach/detach a
completely new view than having to move the existing WebView around.

[Ignacio Solla, 2014-10-10 09:23:41]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;
On 2014/10/10 02:15:25, sievers wrote:
> Can you avoid holding this ref here?

To avoid holding this ref we would need to return a boolean from
FullscreenController::did{Enter|Exit}FullScreen() to tell us whether the element
is a video, and then either propagate it in the IPC or only send the IPC for
non-video elements.

Android is the only platform that needs this, so I would prefer not modifying
blink's API as we can already use this mechanism to detect whether the element
is a video.

[Ignacio Solla, 2014-10-10 09:25:50]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;
On 2014/10/10 09:23:41, Ignacio Solla wrote:
> On 2014/10/10 02:15:25, sievers wrote:
> > Can you avoid holding this ref here?
> 
> To avoid holding this ref we would need to return a boolean from
> FullscreenController::did{Enter|Exit}FullScreen() to tell us whether the
element
> is a video, and then either propagate it in the IPC or only send the IPC for
> non-video elements.
> 
> Android is the only platform that needs this, so I would prefer not modifying
> blink's API as we can already use this mechanism to detect whether the element
> is a video.

Please ignore this comment, I have published it by mistake. It turns out that
there is a way in which we can avoid holding this ref, see the changes or the
other comment.

[Ignacio Solla, 2014-10-10 16:14:35]

On 2014/10/10 09:21:20, Ignacio Solla wrote:
> On 2014/10/10 02:06:15, sievers wrote:
> > Why do we need a fullscreen video view when you go into fullscreen for a
> > non-video element?
> > I thought that normally works through compositing with the UI being hidden.
> 
> In short, because we want to avoid having two different Android APIs for
> fullscreen, one for video and one for non-video.
> 
> By creating a fullscreen view apps that support fullscreen video will start
> supporting fullscreen for non-video transparently without having to perform
any
> changes to their app, as WebChromeClient.{onShow|onHide}CustomView will
already
> work for non-video.
> 
> From the point of view of the app it is probably also simpler to attach/detach
a
> completely new view than having to move the existing WebView around.

Also note that we don't create a fullscreen video view (ContentVideoView) for
non-video. We only create a FullscreenView (for video this renders the html5
controls, for non-video this renders the element in fullscreen). A
FullscreenView is only created in the WebView. Clank behaves as you described.

[no sievers, 2014-10-10 23:00:42]

On 2014/10/10 09:23:41, Ignacio Solla wrote:
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
> Android is the only platform that needs this, so I would prefer not modifying
> blink's API as we can already use this mechanism to detect whether the element
> is a video.

I'm confused. This whole issue is about HTML5 fullscreen, right?
Chrome incl. Chrome for Android supports this, only Android WebView does not it
seems.

This comes from Blink (FullscreenController::enterFullScreenForElement()) and
goes to WebContentsDelegate::ToggleFullscreenModeForTab() in the browser.
android_webview/native/aw_web_contents_delegate.cc seems to implement something
there. The embedder can then hide the UI and resize the view. And you can give
the View to the WebView embedder if you want to do it that way. What's not
working for you with the existing content API that works for all other
embedders?

I would not conflate HTML5 fullscreen with WMPAndroid fullscreen like I *think*
this patch does.
They are different things. The latter one is an Android implementation detail
for DRM (hole punching with the compositor) and maybe a perf optimization for
pure media elements where we can put video directly into an overlay without
compositing.

If WebView exposes wants to expose both features to its embedder in a similar
fashion, that sounds fine. But should be an implementation detail there and not
cause things to get mixed up in the shared code. And it shouldn't expose another
fullscreen API like here in ContentViewClient.

[Ignacio Solla, 2014-10-13 11:33:18]

On 2014/10/10 23:00:42, sievers wrote:
> On 2014/10/10 09:23:41, Ignacio Solla wrote:
> >
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/618013003/diff/60001/content/public/android/j...
> > Android is the only platform that needs this, so I would prefer not
modifying
> > blink's API as we can already use this mechanism to detect whether the
element
> > is a video.
> 
> I'm confused. This whole issue is about HTML5 fullscreen, right?
> Chrome incl. Chrome for Android supports this, only Android WebView does not
it
> seems.
> 
> This comes from Blink (FullscreenController::enterFullScreenForElement()) and
> goes to WebContentsDelegate::ToggleFullscreenModeForTab() in the browser.
> android_webview/native/aw_web_contents_delegate.cc seems to implement
something
> there. The embedder can then hide the UI and resize the view. And you can give
> the View to the WebView embedder if you want to do it that way. What's not
> working for you with the existing content API that works for all other
> embedders?
> 
> I would not conflate HTML5 fullscreen with WMPAndroid fullscreen like I
*think*
> this patch does.
> They are different things. The latter one is an Android implementation detail
> for DRM (hole punching with the compositor) and maybe a perf optimization for
> pure media elements where we can put video directly into an overlay without
> compositing.
> 
> If WebView exposes wants to expose both features to its embedder in a similar
> fashion, that sounds fine. But should be an implementation detail there and
not
> cause things to get mixed up in the shared code. And it shouldn't expose
another
> fullscreen API like here in ContentViewClient.

In short, the existing content API does not notify the embedder about whether we
are
entering fullscreen for a video element or not and the WebView needs to know
this.

First, let me explain the background behind this change and hopefully clear
your confusion. Also, I will propose an alternative solution that I'd be happy
to implement if you prefer.

Currently, the Android WebView does only support fullscreen for video elements. 
Whenever we want to enter/exit fullscreen we notify the app by calling
WebChromeClient.onShowCustomView/onHideCustomView:

http://developer.android.com/reference/android/webkit/WebChromeClient.html

The app then decides where they want to place the custom view, it might occupy
the whole screen, or just a part of it. Note that the WebView does not own the
view
hierarchy so we'are not allowed to modify it (this is different to Clank). In
the
WebView it is up to the app to decide what to do when entering fullscreen.

As you noted, in order to support the MSE/EME extensions we need hardware
acceleration. For
that we create the ContentVideoView. So to enter fullscreen we call
onShowCustomView and pass
a ViewGroup containing the ContentVideoView (to render the video frames) and a
FullscreenView
(to render the html5 video controls).

Now we want to add support for fullscreen for non-video elements and we need to
ensure that:
- the above still applies, only the app can choose where to place the custom
view
- the same API is shared between video and non-video
- support for non-video does not break backwards compatibility
- (nice to have) support for non-video works out of the box and does not require
modifications
  to the apps

Given those constraints we'd like to reuse the existing WebView API. So for
non-video we will need to call onShowCustomView and pass just a FullscreenView
(to render the
non-video element in fullscreen). So in summary, THE WEBVIEW NEEDS TO KNOW
WHETHER WE ARE GOING
FULLSCREEN ON A VIDEO ELEMENT OR NOT BECAUSE:

- for video we need to call onShowCustomView with a ViewGroup containing a
FullscreenView and
  a ContentVideoView
- for non-video we need to call onShowCustomView with just a FullscreenView.

With the existing content API we don't know whether we are going fullscreen on a
video element
or not. I could modify the ToggleFullscreenMode API to take an extra argument
(bool isVideo) as
shown here:
https://codereview.chromium.org/652673002/
https://codereview.chromium.org/646403002/

Let me know if you prefer that solution and I'd happily implement it here. It is
not a clear-cut
decision, but there are reasons why I prefer the approach in this patch. Let me
explain those.

With the current approach only ContentViewCore needs to be aware of the
distinction between
video and non-video. I believe that this makes the code simpler and easier to
understand. It also
makes the order of events consistent:

1. toggleFullscreenMode is called
2. didEnterFullscreenMode is called
3. (only for video) ContentVideoView is created
4. and finally onShowCustomView is called

If instead we modify the ToggleFullscreenMode API then we need to propagate the
isVideo argument
from blink to the embedder, which means that blink, IPCs, etc. also need to be
aware of the distinction.
It also makes the order in which events happen inconsistent between video and
non video:

- for video:

1. ToggleFullscreen is called
2. DidEnterFullscreen is called
3. ContentVideoView is created
3. onShowCustomView is called

- for non-video:

1. ToggleFullscreen is called
2. onShowCustomView is called
3. DidEnterFullscreen is called


So in summary I think that the current approach makes the code simpler. But if
you prefer
I could modify the ToggleFullscreen API instead. A different solution that I
haven't explored is
to create the ContentVideoView before
AwWebContentsDelegate.toggleFullscreenModeForTab is called,
but I suspect that that's not possible because we should only create the
ContentVideoView when
FullscreenController.didEnterFullscreen is called.

Please do let me know if things are clearer now.

[no sievers, 2014-10-13 17:45:46]

Maybe it might be easier to sort this out in chat or or quick video meeting?
I feel like you are still mixing up WebView requirements and decisions with
regard to that API with ones that concern the common code.

The only two requirements I can distill out of this easily are:

1) need to know whether you are entering or exiting fullscreen.
Well all platforms need to know this and they somehow manage with the 'toggle'
API. I assume it's because you can simply cache what state you are in.

2) need to differentiate between the special video fullscreen mode and
video-element only Android-specific optimization.
Chrome for Android manages to do that just fine with the current API.

[Ignacio Solla, 2014-10-13 18:00:16]

On 2014/10/13 17:45:46, sievers wrote:
> Maybe it might be easier to sort this out in chat or or quick video meeting?
> I feel like you are still mixing up WebView requirements and decisions with
> regard to that API with ones that concern the common code.
> 
> The only two requirements I can distill out of this easily are:
> 
> 1) need to know whether you are entering or exiting fullscreen.
> Well all platforms need to know this and they somehow manage with the 'toggle'
> API. I assume it's because you can simply cache what state you are in.
> 
> 2) need to differentiate between the special video fullscreen mode and
> video-element only Android-specific optimization.
> Chrome for Android manages to do that just fine with the current API.

I have invited you to a quick vc meeting tomorrow to discuss. The current
content API does not provide enough information for us to implement the WebView
API. I think that you are mixing Clank requirements with WebView requirements.
WebView requirements are more complex because the WebView framework does not own
the view hiearchy.

[Ignacio Solla, 2014-10-15 11:17:22]

On 2014/10/13 18:00:16, Ignacio Solla wrote:
> On 2014/10/13 17:45:46, sievers wrote:
> > Maybe it might be easier to sort this out in chat or or quick video meeting?
> > I feel like you are still mixing up WebView requirements and decisions with
> > regard to that API with ones that concern the common code.
> > 
> > The only two requirements I can distill out of this easily are:
> > 
> > 1) need to know whether you are entering or exiting fullscreen.
> > Well all platforms need to know this and they somehow manage with the
'toggle'
> > API. I assume it's because you can simply cache what state you are in.
> > 
> > 2) need to differentiate between the special video fullscreen mode and
> > video-element only Android-specific optimization.
> > Chrome for Android manages to do that just fine with the current API.
> 
> I have invited you to a quick vc meeting tomorrow to discuss. The current
> content API does not provide enough information for us to implement the
WebView
> API. I think that you are mixing Clank requirements with WebView requirements.
> WebView requirements are more complex because the WebView framework does not
own
> the view hiearchy.

It turns out that I missed one option that Daniel pointed out. When the browser
receives the ToggleFullscreenModeForTab IPC, we ask the app to show a custom
ViewGroup. If later a ContentVideoView is created then we add it to the custom
ViewGroup. This simplifies things. Thanks Daniel!

Please PTAL.

[no sievers, 2014-10-15 18:16:39]

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:431:
contentViewCore.getContentVideoViewClient());
nit: contentViewCore.getContentVideoViewClient() -> client

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2709:
mWebContents.exitFullscreen();
Is this needed? Doesn't AwContents have access to the WebContents?

We've really been wanting to make ContentViewCore less of a poorly defined
super-blob of API surface (mixed public and private). So for anything that can
simply go through existing functions in WebContents or RWHV or something else,
that'd be much preferred :)

[Ignacio Solla, 2014-10-15 18:38:42]

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:431:
contentViewCore.getContentVideoViewClient());
On 2014/10/15 18:16:38, sievers wrote:
> nit: contentViewCore.getContentVideoViewClient() -> client

Done.

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2709:
mWebContents.exitFullscreen();
On 2014/10/15 18:16:38, sievers wrote:
> Is this needed? Doesn't AwContents have access to the WebContents?
> 
> We've really been wanting to make ContentViewCore less of a poorly defined
> super-blob of API surface (mixed public and private). So for anything that can
> simply go through existing functions in WebContents or RWHV or something else,
> that'd be much preferred :)

This function was already here when I started with this change. When I rebased I
assumed that it had been deleted. I did not realize that it was moved. Using the
version in WebContents now.

[no sievers, 2014-10-15 18:48:21]

lgtm

[Ignacio Solla, 2014-10-16 08:26:20]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-10-16 08:27:14]

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

[commit-bot: I haz the power, 2014-10-16 10:07:30]

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

[commit-bot: I haz the power, 2014-10-16 10:07:31]

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

[Ignacio Solla, 2014-10-16 13:44:19]

https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
(left):

https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:49:
@DisableHardwareAccelerationForTest
FYI: playing fullscreen video in the WebView requires hardware acceleration. See
the documentation in
http://developer.android.com/reference/android/webkit/WebView.html or
b/15021171. 

The reason why this test was passing before is because we have changed the
timing of events. The hardware check only happens when we first try to draw into
the custom ViewGroup. We now try to draw into the custom ViewGroup before
creating the ContentVideoView, the check fails, so we don't create the
ContentVideoView. Before we were creating the ContentVideoView before the first
draw.

[Ignacio Solla, 2014-10-16 13:44:27]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-10-16 13:45:02]

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

[commit-bot: I haz the power, 2014-10-16 14:32:00]

Committed patchset #9 (id:350001)

[commit-bot: I haz the power, 2014-10-16 14:32:41]

Patchset 9 (id:??) landed as
https://crrev.com/3e28946a9edc2d794a894a6a4a0be965f8b62761
Cr-Commit-Position: refs/heads/master@{#299889}

[boliu, 2014-10-16 15:16:07]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2014-10-16 15:16:07]

https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
(left):

https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:49:
@DisableHardwareAccelerationForTest
On 2014/10/16 13:44:19, Ignacio Solla wrote:
> FYI: playing fullscreen video in the WebView requires hardware acceleration.
See
> the documentation in
> http://developer.android.com/reference/android/webkit/WebView.html or
> b/15021171. 
> 
> The reason why this test was passing before is because we have changed the
> timing of events. The hardware check only happens when we first try to draw
into
> the custom ViewGroup. We now try to draw into the custom ViewGroup before
> creating the ContentVideoView, the check fails, so we don't create the
> ContentVideoView. Before we were creating the ContentVideoView before the
first
> draw.

We should add a test that video in software mode doesn't cause deadlocks etc.

[Ignacio Solla, 2014-10-16 15:20:10]

On 2014/10/16 15:16:07, boliu wrote:
>
https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
> (left):
> 
>
https://codereview.chromium.org/618013003/diff/350001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:49:
> @DisableHardwareAccelerationForTest
> On 2014/10/16 13:44:19, Ignacio Solla wrote:
> > FYI: playing fullscreen video in the WebView requires hardware acceleration.
> See
> > the documentation in
> > http://developer.android.com/reference/android/webkit/WebView.html or
> > b/15021171. 
> > 
> > The reason why this test was passing before is because we have changed the
> > timing of events. The hardware check only happens when we first try to draw
> into
> > the custom ViewGroup. We now try to draw into the custom ViewGroup before
> > creating the ContentVideoView, the check fails, so we don't create the
> > ContentVideoView. Before we were creating the ContentVideoView before the
> first
> > draw.
> 
> We should add a test that video in software mode doesn't cause deadlocks etc.

But fullscreen video in software mode is not supported.

[boliu, 2014-10-16 15:27:53]

On 2014/10/16 15:20:10, Ignacio Solla wrote:
> On 2014/10/16 15:16:07, boliu wrote:
> > We should add a test that video in software mode doesn't cause deadlocks
etc.
> 
> But fullscreen video in software mode is not supported.

Not supported doesn't mean we can deadlock in that case.

Not supported here just means the video itself won't show up, and that should be
the only issue. If a page loads a <video> and makes it go to full screen, we
should not have any deadlocks even in software mode. Plus full screen video used
to worked in software mode, ie the video would show up, as that didn't require
reading and compositing a texture, so no need for hardware acceleration.

[Ignacio Solla, 2014-10-16 15:30:33]

On 2014/10/16 15:27:53, boliu wrote:
> On 2014/10/16 15:20:10, Ignacio Solla wrote:
> > On 2014/10/16 15:16:07, boliu wrote:
> > > We should add a test that video in software mode doesn't cause deadlocks
> etc.
> > 
> > But fullscreen video in software mode is not supported.
> 
> Not supported doesn't mean we can deadlock in that case.
> 
> Not supported here just means the video itself won't show up, and that should
be
> the only issue. If a page loads a <video> and makes it go to full screen, we
> should not have any deadlocks even in software mode. Plus full screen video
used
> to worked in software mode, ie the video would show up, as that didn't require
> reading and compositing a texture, so no need for hardware acceleration.

Fair enough. I'll add a test for that in a follow-up patch.