Migrate media::MediaPlayerManager::Create() to content::BrowserMediaPlayerManager.

Migrate media::MediaPlayerManager::Create() to content::BrowserMediaPlayerManager.

This fixes a layering violation where content::RenderViewHost was forward declared in media code. It also lets content code use BrowserMediaPlayerManager directly without resorting to casting.

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

[scherkus (not reviewing), 2013-08-29 18:45:46]

+avayvod for CC

[whywhat, 2013-08-29 18:57:25]

It seems a shame to loose an ability to provide a totally separate
implementation of the media player manager (e.g. instead of the current subclass
or to mock the thing).
I recognize though it's a potential benefit vs. the actual benefit introduced by
this change.

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1306: if
(browser_media_player_manager) {
Nit:
maybe

if (rvhi && rvhi->media_player_manager()) {
  rvhi->media_player_manager()->AttachExternalVideoSurface();
}

will make the code a little shorter and easier to read?

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1320: rvhi ?
rvhi->media_player_manager() : NULL;
Nit: ditto

[qinmin, 2013-08-29 18:58:46]

lgtm % nits

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
File content/browser/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.cc:36:
content::RenderViewHost* rvh) {
ditto

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
File content/browser/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.h:47: static
BrowserMediaPlayerManager* Create(content::RenderViewHost* rvh);
ditto

[qinmin, 2013-08-29 18:59:41]

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
File content/browser/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.h:43: typedef
BrowserMediaPlayerManager* (*Factory)(content::RenderViewHost*);
nit: remove content::

[scherkus (not reviewing), 2013-08-29 19:11:54]

On 2013/08/29 18:57:25, whywhat wrote:
> It seems a shame to loose an ability to provide a totally separate
> implementation of the media player manager (e.g. instead of the current
subclass
> or to mock the thing).
> I recognize though it's a potential benefit vs. the actual benefit introduced
by
> this change.

Yep. Until we absolutely need it, we might as well not deal with unnecessary
additional layers of abstraction.

[scherkus (not reviewing), 2013-08-29 19:12:04]

thanks for the reviews!

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
File content/browser/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.cc:36:
content::RenderViewHost* rvh) {
On 2013/08/29 18:58:46, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
File content/browser/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.h:43: typedef
BrowserMediaPlayerManager* (*Factory)(content::RenderViewHost*);
On 2013/08/29 18:59:41, qinmin wrote:
> nit: remove content::

Done.

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/b...
content/browser/android/browser_media_player_manager.h:47: static
BrowserMediaPlayerManager* Create(content::RenderViewHost* rvh);
On 2013/08/29 18:58:46, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1306: if
(browser_media_player_manager) {
On 2013/08/29 18:57:25, whywhat wrote:
> Nit:
> maybe
> 
> if (rvhi && rvhi->media_player_manager()) {
>   rvhi->media_player_manager()->AttachExternalVideoSurface();
> }
> 
> will make the code a little shorter and easier to read?

Done.

https://codereview.chromium.org/23735002/diff/10001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1320: rvhi ?
rvhi->media_player_manager() : NULL;
On 2013/08/29 18:57:25, whywhat wrote:
> Nit: ditto

Done.

[scherkus (not reviewing), 2013-08-29 19:14:24]

+jam for content/browser/* OWNERS

[jam, 2013-08-29 20:27:24]

lgtm

https://codereview.chromium.org/23735002/diff/17001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23735002/diff/17001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:71: #if
defined(OS_ANDROID)
nit: it's easier to read if you just take out the ifdef. no harm in forward
declaring it for all platforms

[scherkus (not reviewing), 2013-08-29 20:31:58]

https://codereview.chromium.org/23735002/diff/17001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23735002/diff/17001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.h:71: #if
defined(OS_ANDROID)
On 2013/08/29 20:27:25, jam wrote:
> nit: it's easier to read if you just take out the ifdef. no harm in forward
> declaring it for all platforms

Done.

[commit-bot: I haz the power, 2013-08-29 20:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/23735002/30001

[commit-bot: I haz the power, 2013-08-30 06:40:21]

Change committed as 220520