Enables fullscreen subtitle and media control from Blink

Enables fullscreen subtitle and media control from Blink

In detail, this patch does the following things:
- Move the Blink SurfaceView on top of ContentVideoView
- Enable necessary bits on the compositor to make background transparent,
  and enables alpha blending.
- Introduce a new command-line flag to enable the new mode. The old-style
  Java media control are moved to ContentVideoViewLegacy, which is enabled
  by default.

To enable the blink-based media control, do the following:
adb shell
sudo echo chrome --enable-overlay-fullscreen-video-subtitle > /data/local/chrome-command-line

R=
BUG=262945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243743

[qinmin, 2013-09-27 19:39:23]

looks good, just one question

https://codereview.chromium.org/25040002/diff/1/content/browser/android/conte...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/25040002/diff/1/content/browser/android/conte...
content/browser/android/content_view_render_view.cc:92: 
nit: extra blank line

https://codereview.chromium.org/25040002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(left):

https://codereview.chromium.org/25040002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:216:
setBackgroundColor(Color.BLACK);
why we need to change this? if the tab surface view(except controls) is
transparent, then we should have something in the back to show the background
color, right?

[trchen, 2013-09-27 22:38:41]

https://codereview.chromium.org/25040002/diff/1/content/browser/android/conte...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/25040002/diff/1/content/browser/android/conte...
content/browser/android/content_view_render_view.cc:92: 
On 2013/09/27 19:39:23, qinmin wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/25040002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(left):

https://codereview.chromium.org/25040002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:216:
setBackgroundColor(Color.BLACK);
On 2013/09/27 19:39:23, qinmin wrote:
> why we need to change this? if the tab surface view(except controls) is
> transparent, then we should have something in the back to show the background
> color, right?

The black filler has a z-order higher than the Blink content. It feels like an
Android bug to me. (or confusing behavior by design?)

[qinmin, 2013-09-28 00:06:46]

lgtm

[aelias_OOO_until_Jul13, 2013-09-28 00:08:01]

lgtm for content/browser/renderer_host/

[Ted C, 2013-09-30 15:54:35]

seems fine to me, but I just want to make sure we don't miss the chrome test
activity as well.

Does this have any impact on webview?

https://codereview.chromium.org/25040002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/25040002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:759: if
(overlay_video_mode_ == enabled)
Doesn't look like you really need to store this value at this point.

Couldn't you also just do:

if (layer_->contents_opaque() == !enabled)
  return;
layer_->SetContentsOpaque(!enabled);

I guess it doesn't really matter either way though.

https://codereview.chromium.org/25040002/diff/1/content/shell/android/java/sr...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/25040002/diff/1/content/shell/android/java/sr...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:91:
if (mContentViewRenderView == null)
In java land, braces are always required on conditionals.  Unless the
conditional and statement all fit on a single line, which I think they'll do
here just fine.

if (mContentViewRenderView == null) return;

https://codereview.chromium.org/25040002/diff/1/content/shell/android/shell_a...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/25040002/diff/1/content/shell/android/shell_a...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:135:
return new ActivityContentVideoViewClient(ContentShellActivity.this) {
Do you need to do something similar for the ChromiumTestShellActivity?

[trchen, 2013-12-21 01:26:17]

This is an updated version of this patch.
The main change is to keep the old ContentVideoView code to
ContentVideoViewLegacy, and introduced a new option flag to enable the new
implementation.

The default is to disable the new implementation. Developers can enable and test
by adding --enable-overlay-fullscreen-video-subtitle to
/data/local/chrome-command-line.

On 2013/09/30 15:54:35, Ted C wrote:
> seems fine to me, but I just want to make sure we don't miss the chrome test
> activity as well.
> 
> Does this have any impact on webview?

At last we decided not to support WebView in first release until we come up with
a better public API.

>
https://codereview.chromium.org/25040002/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/25040002/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_view_android.cc:759: if
> (overlay_video_mode_ == enabled)
> Doesn't look like you really need to store this value at this point.
> 
> Couldn't you also just do:
> 
> if (layer_->contents_opaque() == !enabled)
>   return;
> layer_->SetContentsOpaque(!enabled);
> 
> I guess it doesn't really matter either way though.

Done

>
https://codereview.chromium.org/25040002/diff/1/content/shell/android/java/sr...
> File
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
> (right):
> 
>
https://codereview.chromium.org/25040002/diff/1/content/shell/android/java/sr...
>
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:91:
> if (mContentViewRenderView == null)
> In java land, braces are always required on conditionals.  Unless the
> conditional and statement all fit on a single line, which I think they'll do
> here just fine.
> 
> if (mContentViewRenderView == null) return;

Done

>
https://codereview.chromium.org/25040002/diff/1/content/shell/android/shell_a...
> File
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/25040002/diff/1/content/shell/android/shell_a...
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:135:
> return new ActivityContentVideoViewClient(ContentShellActivity.this) {
> Do you need to do something similar for the ChromiumTestShellActivity?

Yes, but probably not at this time. Fullscreen video wasn't working in chromium
shell in the first place.
(ContentViewCore returns a null ContentVideoViewClient, which is not expected. I
will look into this as a separate issue.)

[trchen, 2013-12-21 01:29:27]

+joi

Can we add a new command line flag to try out this experimental feature? Thanks!

[qinmin, 2013-12-21 01:35:13]

lgtm % nits

https://codereview.chromium.org/25040002/diff/93001/content/browser/media/and...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/25040002/diff/93001/content/browser/media/and...
content/browser/media/android/browser_media_player_manager.cc:182: if
(RenderWidgetHostViewAndroid* view_android =
nit: add {} for multi-line if statement

https://codereview.chromium.org/25040002/diff/93001/content/browser/media/and...
content/browser/media/android/browser_media_player_manager.cc:217: if
(RenderWidgetHostViewAndroid* view_android =
ditto

[Jói, 2013-12-21 08:54:55]

//content/public (minus Java bits - I think you have a different reviewer for
those) LGTM.

[Ted C, 2013-12-26 23:06:14]

What about ChromiumTestShellActivity?

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:158:
protected void showContentVideoView() {
All non-private methods should have java doc

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:175:
protected SurfaceView getSurfaceView() { return mVideoSurfaceView; }
I would put the return on the subsequent line.  I usually only see the single
line if the method body is empty.

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:367:
private static ContentVideoView createContentVideoViewLegacy(
why is this in this file instead of ContentVideoViewLegacy?  It seems odd to
have a generic superclass refer to a specific child instance.

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:372:
Log.w(TAG, "Wrong type of context, can't create fullscreen video");
should this be an assert?

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:16:
public class ContentVideoViewLegacy extends ContentVideoView {
needs a class level description

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:64:
keyCode != KeyEvent.KEYCODE_VOLUME_UP &&
Although this is moved code, these lines should be indented 8 from the start of
the line instead of lining it up.

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:128:
return;
this can probably all fit on a single line

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:130:
if (mMediaController != null) {
same

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:154:
if (mMediaController != null) {
personally, I prefer to exit early in this case and reduce the indent of the
subsequent block:

if (mMediaController == null) return;

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:157:
if (isPlaying())
braces required in java land

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:181:
if (mMediaController != null) {
make a one liner here too

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:243:
* Enter or leave overlay video mode.
Should have an:

@param enabled Whether overlay mode is enabled.

https://codereview.chromium.org/25040002/diff/93001/content/shell/android/jav...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/shell/android/jav...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:89:
* Enter or leave overlay video mode.
same comment about the missing @param

[trchen, 2013-12-26 23:22:45]

Fullscreen video doesn't work with ChromiumTestShell currently. Will file a bug
to keep track of it.

Thanks for reviewing this! Will fix all the style issues.

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:367:
private static ContentVideoView createContentVideoViewLegacy(
On 2013/12/26 23:06:15, Ted C wrote:
> why is this in this file instead of ContentVideoViewLegacy?  It seems odd to
> have a generic superclass refer to a specific child instance.

Then we'll have to generate another JNI header for ContentVideoViewLegacy, and
unfortunately we can't include both header within the same compiling
unit(content_video_view.cc), so we have to add another C++ wrapper as well.

I'm thinking the better way is to re-use createContentVideoView() above, just
adding a boolean parameter for which version to create.

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:372:
Log.w(TAG, "Wrong type of context, can't create fullscreen video");
On 2013/12/26 23:06:15, Ted C wrote:
> should this be an assert?

I don't know. It is copy & paste from createContentVideoView() above. I'd rather
not touch it.

[Ted C, 2013-12-27 00:15:34]

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:367:
private static ContentVideoView createContentVideoViewLegacy(
On 2013/12/26 23:22:46, trchen wrote:
> On 2013/12/26 23:06:15, Ted C wrote:
> > why is this in this file instead of ContentVideoViewLegacy?  It seems odd to
> > have a generic superclass refer to a specific child instance.
> 
> Then we'll have to generate another JNI header for ContentVideoViewLegacy, and
> unfortunately we can't include both header within the same compiling
> unit(content_video_view.cc), so we have to add another C++ wrapper as well.
> 
> I'm thinking the better way is to re-use createContentVideoView() above, just
> adding a boolean parameter for which version to create.

Do we plan on removing ContentVideoViewLegacy at some point in the future?

If yes, then I think this is a fine stop gap solution.  If this is going to be
long lived, then I think we should probably move to some sort of factory concept
for ContentVideoViews, which sadly will mean a new java/c++ pair.

[trchen, 2013-12-27 00:24:30]

On 2013/12/27 00:15:34, Ted C wrote:
>
https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
> (right):
> 
>
https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:367:
> private static ContentVideoView createContentVideoViewLegacy(
> On 2013/12/26 23:22:46, trchen wrote:
> > On 2013/12/26 23:06:15, Ted C wrote:
> > > why is this in this file instead of ContentVideoViewLegacy?  It seems odd
to
> > > have a generic superclass refer to a specific child instance.
> > 
> > Then we'll have to generate another JNI header for ContentVideoViewLegacy,
and
> > unfortunately we can't include both header within the same compiling
> > unit(content_video_view.cc), so we have to add another C++ wrapper as well.
> > 
> > I'm thinking the better way is to re-use createContentVideoView() above,
just
> > adding a boolean parameter for which version to create.
> 
> Do we plan on removing ContentVideoViewLegacy at some point in the future?
> 
> If yes, then I think this is a fine stop gap solution.  If this is going to be
> long lived, then I think we should probably move to some sort of factory
concept
> for ContentVideoViews, which sadly will mean a new java/c++ pair.

Yes we do plan to remove the legacy mode once the new mode can be shipped in
stable
channel for all clank flavor. Currently we won't be able to support WebView
easily,
that's why we keep the legacy mode.

[Ted C, 2013-12-27 00:51:09]

On 2013/12/27 00:24:30, trchen wrote:
> On 2013/12/27 00:15:34, Ted C wrote:
> >
>
https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/25040002/diff/93001/content/public/android/ja...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:367:
> > private static ContentVideoView createContentVideoViewLegacy(
> > On 2013/12/26 23:22:46, trchen wrote:
> > > On 2013/12/26 23:06:15, Ted C wrote:
> > > > why is this in this file instead of ContentVideoViewLegacy?  It seems
odd
> to
> > > > have a generic superclass refer to a specific child instance.
> > > 
> > > Then we'll have to generate another JNI header for ContentVideoViewLegacy,
> and
> > > unfortunately we can't include both header within the same compiling
> > > unit(content_video_view.cc), so we have to add another C++ wrapper as
well.
> > > 
> > > I'm thinking the better way is to re-use createContentVideoView() above,
> just
> > > adding a boolean parameter for which version to create.
> > 
> > Do we plan on removing ContentVideoViewLegacy at some point in the future?
> > 
> > If yes, then I think this is a fine stop gap solution.  If this is going to
be
> > long lived, then I think we should probably move to some sort of factory
> concept
> > for ContentVideoViews, which sadly will mean a new java/c++ pair.
> 
> Yes we do plan to remove the legacy mode once the new mode can be shipped in
> stable
> channel for all clank flavor. Currently we won't be able to support WebView
> easily,
> that's why we keep the legacy mode.

Can you add a TODO with a link to a bug so we track that this needs to be
removed?

[trchen, 2014-01-07 01:38:21]

This patch is revised as suggested, except that protected method in
ContentVideoView are not commented (they will become private again once we
remove ContentVideoViewLegacy).

Also the back button issue and renderer-initiated exit fullscreen issue are
fixed. I think it is in a shippable shape now.

[Ted C, 2014-01-08 00:52:00]

On 2014/01/07 01:38:21, trchen wrote:
> This patch is revised as suggested, except that protected method in
> ContentVideoView are not commented (they will become private again once we
> remove ContentVideoViewLegacy).
> 
> Also the back button issue and renderer-initiated exit fullscreen issue are
> fixed. I think it is in a shippable shape now.

lgtm

[commit-bot: I haz the power, 2014-01-08 23:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/25040002/333001

[commit-bot: I haz the power, 2014-01-09 02:16:01]

Change committed as 243743