Move WebMediaPlayerImpl and its dependencies to media/blink.

Move WebMediaPlayerImpl and its dependencies to media/blink.

Moving WebMediaPlayerImpl and related classes in content/renderer/media to media/blink so that they can be reused by Mojo code.

BUG=408338
Committed: https://crrev.com/9e0840d0672da96350a1f33d684f2c64d2574f46
Cr-Commit-Position: refs/heads/master@{#293628}

[acolwell GONE FROM CHROMIUM, 2014-09-02 18:33:50]

acolwell@chromium.org changed reviewers:
+ jamesr@chromium.org, scherkus@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-09-02 18:33:52]

jamesr: Please OWNERS review content/renderer/render_frame_impl.*
scherkus: Please review everything.

[jamesr, 2014-09-02 19:26:45]

lgtm

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
content/renderer/media/buffered_data_source_unittest.cc:36: namespace media {
so long as this test lives in content/... it should be in namespace content::.
if you want to move this to the media namespace put it under the media
directory. having symbols in media:: defined here is confusing.

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
File content/renderer/media/buffered_resource_loader_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
content/renderer/media/buffered_resource_loader_unittest.cc:42: namespace media
{
ditto

https://codereview.chromium.org/495353003/diff/200001/media/blink/BUILD.gn
File media/blink/BUILD.gn (right):

https://codereview.chromium.org/495353003/diff/200001/media/blink/BUILD.gn#ne...
media/blink/BUILD.gn:74: "../base/run_all_unittests.cc",
in GN, you can put this in a source_set and then list it in the deps section.
that's more natural than having multiple places list the same file in their
sources list

[acolwell GONE FROM CHROMIUM, 2014-09-02 20:20:25]

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
content/renderer/media/buffered_data_source_unittest.cc:36: namespace media {
On 2014/09/02 19:26:44, jamesr wrote:
> so long as this test lives in content/... it should be in namespace content::.
> if you want to move this to the media namespace put it under the media
> directory. having symbols in media:: defined here is confusing.

I can't move it into media/ because the test relies on fakes and mocks in
content/. This test is also a friend of the BufferedDataSource so it can access
internal state. Putting this in the media namespace seemed like a better
solution then referencing the content namespace from media/ in the friend
declaration. Would a comment explaining this be acceptable or do you want me to
make all the necessary changes to avoid the friend decl?

[scherkus (not reviewing), 2014-09-02 20:29:52]

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
File content/renderer/media/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
content/renderer/media/buffered_data_source_unittest.cc:36: namespace media {
On 2014/09/02 20:20:25, acolwell wrote:
> On 2014/09/02 19:26:44, jamesr wrote:
> > so long as this test lives in content/... it should be in namespace
content::.
> > if you want to move this to the media namespace put it under the media
> > directory. having symbols in media:: defined here is confusing.
> 
> I can't move it into media/ because the test relies on fakes and mocks in
> content/. This test is also a friend of the BufferedDataSource so it can
access
> internal state. Putting this in the media namespace seemed like a better
> solution then referencing the content namespace from media/ in the friend
> declaration. Would a comment explaining this be acceptable or do you want me
to
> make all the necessary changes to avoid the friend decl?

Is moving the mocks an option? AFAICT they're only used by media/ code based on
some git-grepping.

$ git grep -l
"\b(MockWebURLLoader\|MockWebFrameClient\|TestResponseGenerator\b)" -- "*.cc"   
                                                 
content/renderer/media/android/media_info_loader_unittest.cc
content/renderer/media/buffered_data_source_unittest.cc
content/renderer/media/buffered_resource_loader_unittest.cc

It seems like media_info_loader_unittest.cc is the only sticking point, but
that's because we haven't decided to tackle moving Android media code into
media/blink/ just yet. Maybe we can start with just moving MediaInfoLoader?

[jamesr, 2014-09-02 20:32:31]

On 2014/09/02 20:20:25, acolwell wrote:
>
https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
> File content/renderer/media/buffered_data_source_unittest.cc (right):
> 
>
https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
> content/renderer/media/buffered_data_source_unittest.cc:36: namespace media {
> On 2014/09/02 19:26:44, jamesr wrote:
> > so long as this test lives in content/... it should be in namespace
content::.
> > if you want to move this to the media namespace put it under the media
> > directory. having symbols in media:: defined here is confusing.
> 
> I can't move it into media/ because the test relies on fakes and mocks in
> content/. This test is also a friend of the BufferedDataSource so it can
access
> internal state. Putting this in the media namespace seemed like a better
> solution then referencing the content namespace from media/ in the friend
> declaration. Would a comment explaining this be acceptable or do you want me
to
> make all the necessary changes to avoid the friend decl?

Friending a class defined in content/... is just as much a lie as having code in
content/... be in the media:: namespace.  I'd say try to move the mocks or if
you can't do that then make the private state be public since it's used outside
of media/...

[scherkus (not reviewing), 2014-09-02 20:44:20]

On 2014/09/02 20:29:52, scherkus wrote:
>
https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
> File content/renderer/media/buffered_data_source_unittest.cc (right):
> 
>
https://codereview.chromium.org/495353003/diff/200001/content/renderer/media/...
> content/renderer/media/buffered_data_source_unittest.cc:36: namespace media {
> On 2014/09/02 20:20:25, acolwell wrote:
> > On 2014/09/02 19:26:44, jamesr wrote:
> > > so long as this test lives in content/... it should be in namespace
> content::.
> > > if you want to move this to the media namespace put it under the media
> > > directory. having symbols in media:: defined here is confusing.
> > 
> > I can't move it into media/ because the test relies on fakes and mocks in
> > content/. This test is also a friend of the BufferedDataSource so it can
> access
> > internal state. Putting this in the media namespace seemed like a better
> > solution then referencing the content namespace from media/ in the friend
> > declaration. Would a comment explaining this be acceptable or do you want me
> to
> > make all the necessary changes to avoid the friend decl?
> 
> Is moving the mocks an option? AFAICT they're only used by media/ code based
on
> some git-grepping.
> 
> $ git grep -l
> "\b(MockWebURLLoader\|MockWebFrameClient\|TestResponseGenerator\b)" -- "*.cc" 
 
>                                                  
> content/renderer/media/android/media_info_loader_unittest.cc
> content/renderer/media/buffered_data_source_unittest.cc
> content/renderer/media/buffered_resource_loader_unittest.cc
> 
> It seems like media_info_loader_unittest.cc is the only sticking point, but
> that's because we haven't decided to tackle moving Android media code into
> media/blink/ just yet. Maybe we can start with just moving MediaInfoLoader?

... wait a tick. c/r/m/ can depend on m/b/ .. so I think moving the mocks to
m/b/ should be sufficient, right? and we can leave the android code as-is?

[acolwell GONE FROM CHROMIUM, 2014-09-04 22:31:44]

scherkus: PTAL. I've moved the 2 offending tests out of content along with the
few necessary support classes. Most of the bots should also build cleanly now.

I'm just trying to make one of the Android GN bots happy and once
https://codereview.chromium.org/539203002/ lands the
win_chromium_x64_rel_swarming bot should be fixed.

[scherkus (not reviewing), 2014-09-04 23:00:37]

lgtm w/ a whole bunch of nits

https://codereview.chromium.org/495353003/diff/300001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/495353003/diff/300001/BUILD.gn#newcode195
BUILD.gn:195: "//media/blink", # Blocked on blink
nit: looks like they're using two spaces here

https://codereview.chromium.org/495353003/diff/300001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/495353003/diff/300001/content/content_rendere...
content/content_renderer.gypi:16: '../media/blink/media_blink.gyp:media_blink',
looks like this is sorted A->Z

IOW, media/blink < media/media.gyp

https://codereview.chromium.org/495353003/diff/300001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/495353003/diff/300001/content/content_shell.g...
content/content_shell.gypi:54: '../media/blink/media_blink.gyp:media_blink',
ditto

https://codereview.chromium.org/495353003/diff/300001/media/blink/buffered_re...
File media/blink/buffered_resource_loader_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/buffered_re...
media/blink/buffered_resource_loader_unittest.cc:89: new media::MediaLog()));
remove media::

https://codereview.chromium.org/495353003/diff/300001/media/blink/media_blink...
File media/blink/media_blink.gyp (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/media_blink...
media/blink/media_blink.gyp:69: 'target_name': 'media_blink_unittests',
I believe we'll want to get these running on bots as after the dust settles from
thie CL

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
File media/blink/run_all_unittests.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:19: public:
indent one extra space

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:20: virtual ~TestBlinkPlatformSupport();
it's a bit odd how you inline some methods, but not others

as long as this file is internally consistent I'm fine w/ whatever

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:36: {
{ on previous line

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:312: const media::AudioHardwareConfig&
audio_hardware_config_;
remove media::

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
File media/blink/webmediaplayer_util.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
media/blink/webmediaplayer_util.cc:117: kMaxURLScheme + 1);
+ 1 here intentional?

[acolwell GONE FROM CHROMIUM, 2014-09-05 00:23:44]

https://codereview.chromium.org/495353003/diff/300001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/495353003/diff/300001/BUILD.gn#newcode195
BUILD.gn:195: "//media/blink", # Blocked on blink
On 2014/09/04 23:00:36, scherkus wrote:
> nit: looks like they're using two spaces here

Done.

https://codereview.chromium.org/495353003/diff/300001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/495353003/diff/300001/content/content_rendere...
content/content_renderer.gypi:16: '../media/blink/media_blink.gyp:media_blink',
On 2014/09/04 23:00:36, scherkus wrote:
> looks like this is sorted A->Z
> 
> IOW, media/blink < media/media.gyp

Done.

https://codereview.chromium.org/495353003/diff/300001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/495353003/diff/300001/content/content_shell.g...
content/content_shell.gypi:54: '../media/blink/media_blink.gyp:media_blink',
On 2014/09/04 23:00:36, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/495353003/diff/300001/media/blink/buffered_re...
File media/blink/buffered_resource_loader_unittest.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/buffered_re...
media/blink/buffered_resource_loader_unittest.cc:89: new media::MediaLog()));
On 2014/09/04 23:00:36, scherkus wrote:
> remove media::

Done.

https://codereview.chromium.org/495353003/diff/300001/media/blink/media_blink...
File media/blink/media_blink.gyp (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/media_blink...
media/blink/media_blink.gyp:69: 'target_name': 'media_blink_unittests',
On 2014/09/04 23:00:36, scherkus wrote:
> I believe we'll want to get these running on bots as after the dust settles
from
> thie CL

Acknowledged.

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
File media/blink/run_all_unittests.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:19: public:
On 2014/09/04 23:00:36, scherkus wrote:
> indent one extra space

Done.

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:20: virtual ~TestBlinkPlatformSupport();
On 2014/09/04 23:00:36, scherkus wrote:
> it's a bit odd how you inline some methods, but not others
> 
> as long as this file is internally consistent I'm fine w/ whatever

Removed inlines.

https://codereview.chromium.org/495353003/diff/300001/media/blink/run_all_uni...
media/blink/run_all_unittests.cc:36: {
On 2014/09/04 23:00:36, scherkus wrote:
> { on previous line

Done.

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:312: const media::AudioHardwareConfig&
audio_hardware_config_;
On 2014/09/04 23:00:36, scherkus wrote:
> remove media::

Done.

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
File media/blink/webmediaplayer_util.cc (right):

https://codereview.chromium.org/495353003/diff/300001/media/blink/webmediapla...
media/blink/webmediaplayer_util.cc:117: kMaxURLScheme + 1);
On 2014/09/04 23:00:36, scherkus wrote:
> + 1 here intentional?

Yes. This fixes a bug that was caught by Riley's UMA presubmit check.

[acolwell GONE FROM CHROMIUM, 2014-09-05 21:10:19]

acolwell@chromium.org changed reviewers:
+ brettw@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-09-05 21:10:21]

brettw: net OWNERs review for media/blink/DEPS please.

[acolwell GONE FROM CHROMIUM, 2014-09-05 21:15:10]

On 2014/09/05 21:10:21, acolwell wrote:
> brettw: net OWNERs review for media/blink/DEPS please.
Oops. Misread the presubmit. 

brettw: please OWNERS review top level BUILD.gn change.

[acolwell GONE FROM CHROMIUM, 2014-09-05 21:17:00]

acolwell@chromium.org changed reviewers:
+ mef@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-09-05 21:17:01]

mef: Please OWNERS review net/ dependencies in media/blink/DEPS

[mef, 2014-09-05 21:25:52]

lgtm

[brettw, 2014-09-05 23:26:39]

BUILD.gn lgtm

[acolwell GONE FROM CHROMIUM, 2014-09-06 00:47:41]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-06 00:49:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/495353003/420001

[commit-bot: I haz the power, 2014-09-06 12:27:31]

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

[commit-bot: I haz the power, 2014-09-06 12:27:33]

Exceeded time limit waiting for builds to trigger.

[acolwell GONE FROM CHROMIUM, 2014-09-06 18:59:16]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-09-06 19:00:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/495353003/420001

[commit-bot: I haz the power, 2014-09-06 19:05:05]

Committed patchset #22 (id:420001) as e934ba664c93b6876065e4f177cd11a9391985ad

[commit-bot: I haz the power, 2014-09-10 03:43:07]

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