Move ExternalVideoSurfaceHolder from WebContents UserData to BrowserMediaPlayerManager.

Move ExternalVideoSurfaceHolder from WebContents UserData to BrowserMediaPlayerManager.

Static functions in ExternalVideoSurfaceContainer hinder from building and linking
'content' by itself.  So I moved the creation of ExternalVideoSurfaceContainer into
ContentBrowserClient and its storage into BrowserMediaPlayerManager.

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

[boliu, 2014-04-11 01:52:09]

Mostly looks good.

+tedchoc and qinmin for android/media things in content
+jam for opinion on content_browser_client

https://codereview.chromium.org/231933006/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/231933006/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_content_browser_client.cc:506:
AwContentBrowserClient::CreateExternalVideoSurfaceContainer(
nit: No indent

https://codereview.chromium.org/231933006/diff/1/android_webview/lib/main/aw_...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/231933006/diff/1/android_webview/lib/main/aw_...
android_webview/lib/main/aw_main_delegate.cc:153:
AwMainDelegate::CreateExternalVideoSurfaceContainer(
nit: No indent.

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:441: void
BrowserMediaPlayerManager::OnFrameInfoUpdated() {
How much of the new code in this file need to be behind VIDEO_HOLE?

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:460:
external_video_surface_container_->RequestExternalVideoSurface(
Need to handle the case when  CreateExternalVideoSurfaceContainer returns null.

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

https://codereview.chromium.org/231933006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:922:
RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(
This cast is not safe, there are cases where the host_ pointer is not a
RenderViewHostImpl (maybe not in android, but certainly true for other
platforms).

I think you can check host_->IsRenderView() before casting.

The usage example above in SynchronousFrameMetadata should be fixed too
(separate patch!)

https://codereview.chromium.org/231933006/diff/1/content/public/browser/conte...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/231933006/diff/1/content/public/browser/conte...
content/public/browser/content_browser_client.cc:320:
ContentBrowserClient::CreateExternalVideoSurfaceContainer(
nit: No indent on this line (see Line 46 above)

[ycheo (away), 2014-04-11 05:05:45]

https://codereview.chromium.org/231933006/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/231933006/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_content_browser_client.cc:506:
AwContentBrowserClient::CreateExternalVideoSurfaceContainer(
On 2014/04/11 01:52:10, boliu wrote:
> nit: No indent

Done.

https://codereview.chromium.org/231933006/diff/1/android_webview/lib/main/aw_...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/231933006/diff/1/android_webview/lib/main/aw_...
android_webview/lib/main/aw_main_delegate.cc:153:
AwMainDelegate::CreateExternalVideoSurfaceContainer(
On 2014/04/11 01:52:10, boliu wrote:
> nit: No indent.

Done.

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:441: void
BrowserMediaPlayerManager::OnFrameInfoUpdated() {
On 2014/04/11 01:52:10, boliu wrote:
> How much of the new code in this file need to be behind VIDEO_HOLE?

The remained parts are to control to enable or disable the hole-punching.
And it's just checking the boolean flag.

https://codereview.chromium.org/231933006/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:460:
external_video_surface_container_->RequestExternalVideoSurface(
On 2014/04/11 01:52:10, boliu wrote:
> Need to handle the case when  CreateExternalVideoSurfaceContainer returns
null.

Done.

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

https://codereview.chromium.org/231933006/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:922:
RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(
On 2014/04/11 01:52:10, boliu wrote:
> This cast is not safe, there are cases where the host_ pointer is not a
> RenderViewHostImpl (maybe not in android, but certainly true for other
> platforms).
> 
> I think you can check host_->IsRenderView() before casting.
> 
> The usage example above in SynchronousFrameMetadata should be fixed too
> (separate patch!)

Done.

[ycheo (away), 2014-04-11 05:06:12]

https://codereview.chromium.org/231933006/diff/1/content/public/browser/conte...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/231933006/diff/1/content/public/browser/conte...
content/public/browser/content_browser_client.cc:320:
ContentBrowserClient::CreateExternalVideoSurfaceContainer(
On 2014/04/11 01:52:10, boliu wrote:
> nit: No indent on this line (see Line 46 above)

Done.

[jam, 2014-04-11 17:41:14]

On 2014/04/11 01:52:09, boliu wrote:
> Mostly looks good.
> 
> +tedchoc and qinmin for android/media things in content
> +jam for opinion on content_browser_client
lgtm

thanks for cleaning this up, we should definitely not have any static methods
declared in content and defined outside of it. do you know of any other
instances?

[Ted C, 2014-04-11 17:50:10]

lgtm w/ some small comments

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:442: if
(external_video_surface_container_) {
braces aren't needed here

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:460: if
(external_video_surface_container_) {
Can this ever be NULL at this point?  If yes, can you state that on the API for
CreateExternalVideoSurfaceContainer.

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:469: if
(external_video_surface_container_) {
Move into the block above?

Or remove if it can't be NULL per the comment above.

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:908: if
(content_view_core_) {
At this point, I would change this method to do:

if (!content_view_core_)
  return;

Then decrease the indent of the rest of the method.

[boliu, 2014-04-11 17:51:49]

On 2014/04/11 17:41:14, jam wrote:
> On 2014/04/11 01:52:09, boliu wrote:
> > Mostly looks good.
> > 
> > +tedchoc and qinmin for android/media things in content
> > +jam for opinion on content_browser_client
> lgtm
> 
> thanks for cleaning this up, we should definitely not have any static methods
> declared in content and defined outside of it. do you know of any other
> instances?

I think that's it.

It was my fault that I missed the static methods during the review when they was
added: https://codereview.chromium.org/132233042/ Bots didn't never caught it
because everything was behind VIDEO_HOLE which at the time nothing in chromium
defined. Ran into this when we are actually trying to enable VIDEO_HOLE for
android.

[jam, 2014-04-11 17:56:42]

https://codereview.chromium.org/231933006/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/231933006/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:653: virtual
ExternalVideoSurfaceContainer* CreateExternalVideoSurfaceContainer(
nit: the convention is to begin these methods with Override, i.e.
OverrideExternalVideoSurfaceContainer

[qinmin, 2014-04-11 17:58:47]

lgtm % nit

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:922:
RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(
nit: indent by 2 spaces

[ycheo (away), 2014-04-14 04:35:17]

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:442: if
(external_video_surface_container_) {
On 2014/04/11 17:50:10, Ted C wrote:
> braces aren't needed here

Done.

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:460: if
(external_video_surface_container_) {
On 2014/04/11 17:50:10, Ted C wrote:
> Can this ever be NULL at this point?  If yes, can you state that on the API
for
> CreateExternalVideoSurfaceContainer.

Modified the api description.

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:469: if
(external_video_surface_container_) {
On 2014/04/11 17:50:10, Ted C wrote:
> Move into the block above?
> 
> Or remove if it can't be NULL per the comment above.
No, the following statement can be executed in case of '!is_request' too.

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:908: if
(content_view_core_) {
On 2014/04/11 17:50:10, Ted C wrote:
> At this point, I would change this method to do:
> 
> if (!content_view_core_)
>   return;
> 
> Then decrease the indent of the rest of the method.

Done.

https://codereview.chromium.org/231933006/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:922:
RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(
On 2014/04/11 17:58:48, qinmin wrote:
> nit: indent by 2 spaces

Done.

https://codereview.chromium.org/231933006/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/231933006/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:653: virtual
ExternalVideoSurfaceContainer* CreateExternalVideoSurfaceContainer(
On 2014/04/11 17:56:42, jam wrote:
> nit: the convention is to begin these methods with Override, i.e.
> OverrideExternalVideoSurfaceContainer

Done.

[boliu, 2014-04-14 04:55:35]

aw lgtm

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:469: if
(external_video_surface_container_) {
On 2014/04/14 04:35:17, Yuncheol Heo wrote:
> On 2014/04/11 17:50:10, Ted C wrote:
> > Move into the block above?
> > 
> > Or remove if it can't be NULL per the comment above.
> No, the following statement can be executed in case of '!is_request' too.

Maybe everything in the is_request block should be factored out into a helper
method instead.

https://codereview.chromium.org/231933006/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/231933006/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:652: // implementation.  The
embedder can return NULL if they're not interested.
"not interested" is vague.

Say something like "Return NULL to disable external surface video."

[ycheo (away), 2014-04-14 13:25:03]

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/231933006/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:469: if
(external_video_surface_container_) {
On 2014/04/14 04:55:36, boliu wrote:
> On 2014/04/14 04:35:17, Yuncheol Heo wrote:
> > On 2014/04/11 17:50:10, Ted C wrote:
> > > Move into the block above?
> > > 
> > > Or remove if it can't be NULL per the comment above.
> > No, the following statement can be executed in case of '!is_request' too.
> 
> Maybe everything in the is_request block should be factored out into a helper
> method instead.

https://codereview.chromium.org/231933006/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/231933006/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.h:652: // implementation.  The
embedder can return NULL if they're not interested.
On 2014/04/14 04:55:36, boliu wrote:
> "not interested" is vague.
> 
> Say something like "Return NULL to disable external surface video."

Done.

[ycheo (away), 2014-04-15 04:58:55]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-04-15 05:00:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/231933006/70001

[commit-bot: I haz the power, 2014-04-15 11:55:12]

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

[commit-bot: I haz the power, 2014-04-15 11:55:12]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[ycheo (away), 2014-04-15 13:20:14]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-04-15 13:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/231933006/70001

[commit-bot: I haz the power, 2014-04-15 16:28:41]

Change committed as 263868