Migrate webkit/renderer/media/ to content/renderer/media/.

Update content API in preparation for migrating webkit/renderer/media/ to content/renderer/media/.

The biggest change is to replace the coarse-grained WebMediaPlayer-based content API with finer-grained APIs for controlling media resource loads (e.g., prerendering) and media stream audio/video rendering (e.g., WebRTC layout tests).

BUG=239826, 251306
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209797

[scherkus (not reviewing), 2013-06-27 18:47:48]

ddorwin: look at everything
jam: look at my DEPS stuff + offer guidance

I'm not overly familiar with the code that introduced the dependencies on
content-internal objects. Going to track down the OWNERS to see what we can do
(e.g., prerendering code)

[jam, 2013-06-27 21:22:51]

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEPS
File chrome/renderer/prerender/DEPS (right):

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEP...
chrome/renderer/prerender/DEPS:4:
"+content/renderer/media/webmediaplayer_impl.h",
can't do this.. chrome can't include anything internal from content even
temporarily. please see the content api wiki
http://www.chromium.org/developers/content-module/content-api

since we don't have implementation inheritance in the content api, how we
usually solve this is moving to containment instead

[scherkus (not reviewing), 2013-06-27 21:29:30]

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEPS
File chrome/renderer/prerender/DEPS (right):

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEP...
chrome/renderer/prerender/DEPS:4:
"+content/renderer/media/webmediaplayer_impl.h",
On 2013/06/27 21:22:51, jam wrote:
> can't do this.. chrome can't include anything internal from content even
> temporarily. please see the content api wiki
> http://www.chromium.org/developers/content-module/content-api
> 
> since we don't have implementation inheritance in the content api, how we
> usually solve this is moving to containment instead

Can you give an example?

Do you mean wrapping a WebKit::WebMediaPlayer object + reimplementing the
interface + forwarding all the methods?

As an aside ... do you know why this scrutiny wasn't applied to anything under
src/webkit/ during content-ification of chrome? Was it out of scope?

[scherkus (not reviewing), 2013-06-27 21:56:50]

https://codereview.chromium.org/18123002/diff/1/content/shell/renderer/DEPS
File content/shell/renderer/DEPS (right):

https://codereview.chromium.org/18123002/diff/1/content/shell/renderer/DEPS#n...
content/shell/renderer/DEPS:4: # on internal content objects
http://crbug.com/239826
jam: I'm assuming these aren't permitted either?

some of these are interfaces (e.g., VideoFrameProvider, MediaStreamClient) -- is
it acceptable to move them to content/public/renderer/ ?

[jam, 2013-06-28 00:31:58]

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEPS
File chrome/renderer/prerender/DEPS (right):

https://codereview.chromium.org/18123002/diff/1/chrome/renderer/prerender/DEP...
chrome/renderer/prerender/DEPS:4:
"+content/renderer/media/webmediaplayer_impl.h",
On 2013/06/27 21:29:31, scherkus wrote:
> On 2013/06/27 21:22:51, jam wrote:
> > can't do this.. chrome can't include anything internal from content even
> > temporarily. please see the content api wiki
> > http://www.chromium.org/developers/content-module/content-api
> > 
> > since we don't have implementation inheritance in the content api, how we
> > usually solve this is moving to containment instead
> 
> Can you give an example?
> 
> Do you mean wrapping a WebKit::WebMediaPlayer object + reimplementing the
> interface + forwarding all the methods?

what i mean is that the prerendering code doesn't implement the interface, it
inherits from the implementation. we don't allow implementation inheritance in
the content api, since that would mean exposing internal implementation details
of content.

it's best to study that code to see exactly what prerendering needs to do. there
may be simpler ways of achieving this.
> 
> As an aside ... do you know why this scrutiny wasn't applied to anything under
> src/webkit/ during content-ification of chrome? Was it out of scope?

the chrome/content split was done for code in src/chrome. we didn't touch other
top level directories like wekbit, net etc.

https://codereview.chromium.org/18123002/diff/1/content/shell/renderer/DEPS
File content/shell/renderer/DEPS (right):

https://codereview.chromium.org/18123002/diff/1/content/shell/renderer/DEPS#n...
content/shell/renderer/DEPS:4: # on internal content objects
http://crbug.com/239826
On 2013/06/27 21:56:50, scherkus wrote:
> jam: I'm assuming these aren't permitted either?

correct

> 
> some of these are interfaces (e.g., VideoFrameProvider, MediaStreamClient) --
is
> it acceptable to move them to content/public/renderer/ ?

sure if an interface is needed to be implemented by an embedder, or to be called
by an embedder, we can move it. but we should only expose what's necessary for
the embedder

[scherkus (not reviewing), 2013-06-28 06:04:41]

PTAL

I introduced some finer-grained APIs for handling the two major content embedder
cases:
  1) chrome prerendering of HTML5 media elements
  2) content_shell stubbing of various WebRTC components for layout tests

To minimize the # of two-sided changes required, I'm opting to keep one set of
temporary DEPS around until https://codereview.chromium.org/18130006 can land +
roll.

https://codereview.chromium.org/18123002/diff/14001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

https://codereview.chromium.org/18123002/diff/14001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_webmediaplayer.h:16: class
PrerenderWebMediaPlayer : public content::RenderViewObserver,
FYI I'm happy to rename this after we agree on the correct approach and name for
MPLD

given it's in a prerender namespace it could even be MPLDImpl, especially
considering there are no other impls

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:127: virtual
MediaPlayerLoadDelegate* OverrideCreateMediaPlayerLoadDelegate(
I'm not a content API pro by any means ... but this seems a bit strange that
every other method deals with WebKit:: interfaces yet I'm introducing two
content:: interface overrides here.

Am I Doing It Wrong(tm) or is this an OK place to put these methods? Is there a
better location for overriding content-ish objects as opposed to WebKit-ish
objects?

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
File content/public/renderer/media_player_load_delegate.h (right):

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
content/public/renderer/media_player_load_delegate.h:14: class CONTENT_EXPORT
MediaPlayerLoadDelegate {
open to suggestions on naming

it's a bit unwieldy as is

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
content/public/renderer/media_stream_audio_renderer.h:13: class
MediaStreamAudioRenderer
FYI now a public content interface

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
File content/public/renderer/media_stream_client.h (right):

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/m...
content/public/renderer/media_stream_client.h:20: class MediaStreamClient {
FYI now a public content interface

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/v...
File content/public/renderer/video_frame_provider.h (right):

https://codereview.chromium.org/18123002/diff/14001/content/public/renderer/v...
content/public/renderer/video_frame_provider.h:20: class CONTENT_EXPORT
VideoFrameProvider
FYI now a public content interface

https://codereview.chromium.org/18123002/diff/14001/content/shell/renderer/DEPS
File content/shell/renderer/DEPS (right):

https://codereview.chromium.org/18123002/diff/14001/content/shell/renderer/DE...
content/shell/renderer/DEPS:2: # rolls into Chromium.
removing all this related code results in a compile failure since
WebTestDelegate defines its methods as pure virtual, so for now I'm leaning
towards having this until that CL lands + gets rolled in

[jam, 2013-06-28 19:06:36]

can you reduce this change to make it more reviewable? i.e. don't do any file
moves for now, and put the new interfaces in webkit/

[scherkus (not reviewing), 2013-06-28 20:18:45]

omg you're killing me here!!! PTAL.

[jam, 2013-06-28 21:59:40]

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:130: virtual
webkit_media::MediaLoadDelegate* OverrideCreateMediaLoadDelegate(
nit: rename to reflect that it's a delegate. the convention is GetFooDelegate()

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:146: virtual
webkit_media::MediaStreamClient* OverrideCreateMediaStreamClient();
I don't see this being called?

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
File webkit/renderer/media/media_load_delegate.h (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
webkit/renderer/media/media_load_delegate.h:15: MediaLoadDelegate() {}
nit: no constructor needed in the interface (see other content api interfaces)

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
webkit/renderer/media/media_load_delegate.h:20: virtual void DeferLoad(const
base::Closure& closure) = 0;
actually, if this is an interface with only one method, then it shouldn't be an
interface. it should just be one callback in ContentRendererClient (see the
content api wiki)

[ddorwin, 2013-06-29 01:38:29]

media LGTM. Thanks!

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:130: virtual
webkit_media::MediaLoadDelegate* OverrideCreateMediaLoadDelegate(
nit: Is there always a delegate or does this optionally create one to be used?
It seems like this should be CreateMediaLoadDelegate or CreateMediaLoadOverride.
This matches with the other methods, though.

https://codereview.chromium.org/18123002/diff/23001/content/shell/renderer/sh...
File content/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/18123002/diff/23001/content/shell/renderer/sh...
content/shell/renderer/shell_content_renderer_client.cc:148: if
(!shell_media_stream_client_)
Is there a reason this would be called multiple times?

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.cc:247: load(url, NULL, cors_mode);
overloading not allowed?

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.cc:251: WebKit::WebMediaSource*
media_source,
nit: Should be last parameter? (Fine to change later.)

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
File webkit/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.h:210: void ContinueLoad(const
WebKit::WebURL& url,
nit: "Continue" implies there was a previous step. "Perform" (aka Do)?

[scherkus (not reviewing), 2013-07-02 01:46:22]

jam: PTAL

hopefully I understood your request to use callbacks in ContentRendererClient
correctly -- I didn't any examples in the content/public/ directory that were
similar to my use case.

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:130: virtual
webkit_media::MediaLoadDelegate* OverrideCreateMediaLoadDelegate(
On 2013/06/28 21:59:40, jam wrote:
> nit: rename to reflect that it's a delegate. the convention is
GetFooDelegate()

Done.

https://codereview.chromium.org/18123002/diff/23001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:146: virtual
webkit_media::MediaStreamClient* OverrideCreateMediaStreamClient();
On 2013/06/28 21:59:40, jam wrote:
> I don't see this being called?

Whoops! Got left out of my reduced CL.

https://codereview.chromium.org/18123002/diff/23001/content/shell/renderer/sh...
File content/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/18123002/diff/23001/content/shell/renderer/sh...
content/shell/renderer/shell_content_renderer_client.cc:148: if
(!shell_media_stream_client_)
On 2013/06/29 01:38:30, ddorwin wrote:
> Is there a reason this would be called multiple times?

gets called every time we're creating a MediaStream-based WebMediaPlayer

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
File webkit/renderer/media/media_load_delegate.h (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
webkit/renderer/media/media_load_delegate.h:15: MediaLoadDelegate() {}
On 2013/06/28 21:59:40, jam wrote:
> nit: no constructor needed in the interface (see other content api interfaces)

Done.

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/med...
webkit/renderer/media/media_load_delegate.h:20: virtual void DeferLoad(const
base::Closure& closure) = 0;
On 2013/06/28 21:59:40, jam wrote:
> actually, if this is an interface with only one method, then it shouldn't be
an
> interface. it should just be one callback in ContentRendererClient (see the
> content api wiki)

I'm not 100% sure what you're referring to ... so I gave this a shot. Let me
know if I'm completely off the mark.

Unfortunately webkit_media code can't directly call the ContentRendererClient
(layering violation!), so that code gets a bit hairy by passing in a
callback-to-a-function-accepting-a-closure... but I can clean it up after things
move.

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.cc:247: load(url, NULL, cors_mode);
On 2013/06/29 01:38:30, ddorwin wrote:
> overloading not allowed?

this is an interface defined in WK

AFAIK it's OK there but in any case that's beyond the scope of this CL

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.cc:251: WebKit::WebMediaSource*
media_source,
On 2013/06/29 01:38:30, ddorwin wrote:
> nit: Should be last parameter? (Fine to change later.)

ditto

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
File webkit/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/18123002/diff/23001/webkit/renderer/media/web...
webkit/renderer/media/webmediaplayer_impl.h:210: void ContinueLoad(const
WebKit::WebURL& url,
On 2013/06/29 01:38:30, ddorwin wrote:
> nit: "Continue" implies there was a previous step. "Perform" (aka Do)?

Done.

[jam, 2013-07-02 15:18:34]

lgtm

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:482:
prerender_player->DeferLoad(closure);
it seems unnecessary to have DeferLoad, why not just pass it in as a parameter
to the constructor? then you can simply have

new prerender::PrerenderWebMediaPlayer(render_view, closure);

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_webmediaplayer.h:15: class
PrerenderWebMediaPlayer : public content::RenderViewObserver {
nit: in this change or another, this class and filename need to be updated to
reflect the change in this class

https://codereview.chromium.org/18123002/diff/55001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/55001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:128: // can run |closure|
immediately if they never wish to defer media resource
nit: "if they never wish" to "if they don't wish" since they can change the
decision later..

[scherkus (not reviewing), 2013-07-02 18:36:18]

thanks for putting up with my hefty CL, jam!

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:482:
prerender_player->DeferLoad(closure);
On 2013/07/02 15:18:34, jam wrote:
> it seems unnecessary to have DeferLoad, why not just pass it in as a parameter
> to the constructor? then you can simply have
> 
> new prerender::PrerenderWebMediaPlayer(render_view, closure);

Doh. Right. Good call!

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

https://codereview.chromium.org/18123002/diff/55001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_webmediaplayer.h:15: class
PrerenderWebMediaPlayer : public content::RenderViewObserver {
On 2013/07/02 15:18:34, jam wrote:
> nit: in this change or another, this class and filename need to be updated to
> reflect the change in this class

yep -- added TODO and will land that as a follow up CL

https://codereview.chromium.org/18123002/diff/55001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18123002/diff/55001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:128: // can run |closure|
immediately if they never wish to defer media resource
On 2013/07/02 15:18:34, jam wrote:
> nit: "if they never wish" to "if they don't wish" since they can change the
> decision later..

Done.

[commit-bot: I haz the power, 2013-07-02 18:58:18]

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

[commit-bot: I haz the power, 2013-07-02 18:58:25]

Failed to apply patch for content/content_shell.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/content_shell.gypi
  Hunk #1 FAILED at 93.
  1 out of 1 hunk FAILED -- saving rejects to file
content/content_shell.gypi.rej

Patch:       content/content_shell.gypi
Index: content/content_shell.gypi
diff --git a/content/content_shell.gypi b/content/content_shell.gypi
index
e619b1c8fec47caf7c50da9b0d3a0a45525eadb1..ae79d0a8a74443952f8984810241c710b370c3b9
100644
--- a/content/content_shell.gypi
+++ b/content/content_shell.gypi
@@ -93,6 +93,8 @@
         'shell/renderer/shell_media_stream_client.h',
         'shell/renderer/shell_render_process_observer.cc',
         'shell/renderer/shell_render_process_observer.h',
+        'shell/renderer/shell_video_frame_provider.cc',
+        'shell/renderer/shell_video_frame_provider.h',
         'shell/renderer/webkit_test_runner.cc',
         'shell/renderer/webkit_test_runner.h',
         'shell/shell.cc',

[commit-bot: I haz the power, 2013-07-02 19:44:16]

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

[commit-bot: I haz the power, 2013-07-02 22:25:56]

Change committed as 209797

[scherkus (not reviewing), 2013-07-03 01:01:00]

https://codereview.chromium.org/18123002/diff/71004/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

https://codereview.chromium.org/18123002/diff/71004/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_webmediaplayer.h:16: // TODO(scherkus):
Rename as this class no longer inherits WebMediaPlayer.
FYI this TODO is being addressed in https://codereview.chromium.org/18587003/

[ycheo (away), 2013-07-03 02:41:03]

https://codereview.chromium.org/18123002/diff/71004/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18123002/diff/71004/content/renderer/render_v...
content/renderer/render_view_impl.cc:2868: EnsureMediaStreamImpl();
QQ) Is this needed?
If it is, should we use 'media_stream_impl_' in L2870?

[scherkus (not reviewing), 2013-07-03 20:10:30]

https://codereview.chromium.org/18123002/diff/71004/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18123002/diff/71004/content/renderer/render_v...
content/renderer/render_view_impl.cc:2868: EnsureMediaStreamImpl();
On 2013/07/03 02:41:04, Yuncheol Heo wrote:
> QQ) Is this needed?
> If it is, should we use 'media_stream_impl_' in L2870?

no it is no longer needed