Move creation of MediaStream renders from MediaStreamImpl to MediaStreamRenderFactory

Remove the use of MediaStreamClient and move creation of MediaStream specific renderers to MediaStreamRendererFactory.

This cleans up RenderFrameImpl as well as moving renderer code out of MediaStreamImpl.
 
MediaStreamImpl should in the end only implement methods necessary for blink::WebUserMediaClient and be renamed to UserMediaClientImpl.


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

[perkj_chrome, 2014-05-26 09:54:12]

xians- can you review the /media code and take a look and render_frame_impl.cc? 

jochen - would you mind helping me with render_view_impl and the layout tests? 

This currently breaks to layout tests since no fake video render is created -
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9493


I think we have better tests than these in content_browsertests. So can they
simply be removed or should do I still need to keep something in
render_view_impl to tell it that it needs to create a fake renderer or should I
try to override createMediaPlayer in WebTestProxyBase ?

[jochen (gone - plz use gerrit), 2014-05-26 14:44:59]

On 2014/05/26 09:54:12, perkj wrote:
> xians- can you review the /media code and take a look and
render_frame_impl.cc? 
> 
> jochen - would you mind helping me with render_view_impl and the layout tests?

> 
> This currently breaks to layout tests since no fake video render is created -
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9493
> 
> 
> I think we have better tests than these in content_browsertests. So can they
> simply be removed or should do I still need to keep something in
> render_view_impl to tell it that it needs to create a fake renderer or should
I
> try to override createMediaPlayer in WebTestProxyBase ?

it would be nice to keep the ability to test e.g. the events in layout tests, so
if it's possible to keep the ability to mock the media player that would be
great

[jochen (gone - plz use gerrit), 2014-05-27 07:26:33]

forgot to publish my drafts

https://codereview.chromium.org/294043015/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/294043015/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:56: const blink::WebUserMediaRequest&
user_media_request) OVERRIDE;
we shouldn't use OVERRIDE on implementations of the blink API. It'll just make
updating the API more difficult

[perkj_chrome, 2014-05-27 11:04:21]

ok- So I fixed the layouttests. I hope this way is acceptable. 

Can both of you please take another look?

https://codereview.chromium.org/294043015/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/294043015/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_impl.h:56: const blink::WebUserMediaRequest&
user_media_request) OVERRIDE;
On 2014/05/27 07:26:33, jochen wrote:
> we shouldn't use OVERRIDE on implementations of the blink API. It'll just make
> updating the API more difficult

Done.

[no longer working on chromium, 2014-05-27 12:30:35]

most of them are nits, but please address them. lgtm

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:142: bool AreAllSourcesRemoved()
const { return sources_.empty(); }
change this name here to are_all_sources_removed() or simply has_no_source() ?

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_renderer_factory.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_renderer_factory.h:40: }  // namespace
content
nit, add an empty line

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.h:14: #include
"content/renderer/media/media_stream_renderer_factory.h"
why not forward the declaration?

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3426: #if defined(ENABLE_WEBRTC)
can you improve the #ifs here?
like 

#if !defined(ENABLE_WEBRTC)
  return NULL
#endif

#if defined(OS_ANDROID) && defined(ARCH_CPU_ARMEL)
...
#endif

// WebRtc is enabled.
return new WebMediaPlayerMS(frame_, client, weak_factory_.GetWeakPtr(),

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3442:
scoped_ptr<MediaStreamRendererFactory> factory(
nit, simply do
return scoped_ptr<MediaStreamRendererFactory>(new MediaStreamRendererFactory());

https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
content/shell/renderer/test_runner/web_frame_test_proxy.h:278: return
factory.Pass();
ditto

[jochen (gone - plz use gerrit), 2014-05-27 12:59:43]

lgtm

[perkj_chrome, 2014-05-27 13:27:03]

xians- want to take another look?

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:142: bool AreAllSourcesRemoved()
const { return sources_.empty(); }
On 2014/05/27 12:30:36, xians1 wrote:
> change this name here to are_all_sources_removed() or simply has_no_source() ?
If you don't mind I will not touch this more. I just removed the semicolon to
pass lint.

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_renderer_factory.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_renderer_factory.h:40: }  // namespace
content
On 2014/05/27 12:30:36, xians1 wrote:
> nit, add an empty line

Done.

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.h:14: #include
"content/renderer/media/media_stream_renderer_factory.h"
On 2014/05/27 12:30:36, xians1 wrote:
> why not forward the declaration?

Done.

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3426: #if defined(ENABLE_WEBRTC)
On 2014/05/27 12:30:36, xians1 wrote:
> can you improve the #ifs here?
> like 
> 
> #if !defined(ENABLE_WEBRTC)
>   return NULL
> #endif
> 
> #if defined(OS_ANDROID) && defined(ARCH_CPU_ARMEL)
> ...
> #endif
> 
> // WebRtc is enabled.
> return new WebMediaPlayerMS(frame_, client, weak_factory_.GetWeakPtr(),

Possibly. But it means that the WebMediaPlayerMS must be compiled even if WEBRTC
is disabled. So I would like to not change this.

https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:3442:
scoped_ptr<MediaStreamRendererFactory> factory(
On 2014/05/27 12:30:36, xians1 wrote:
> nit, simply do
> return scoped_ptr<MediaStreamRendererFactory>(new
MediaStreamRendererFactory());

Done.

https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):

https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
content/shell/renderer/test_runner/web_frame_test_proxy.h:278: return
factory.Pass();
On 2014/05/27 12:30:36, xians1 wrote:
> ditto

Done.

[no longer working on chromium, 2014-05-27 15:25:58]

On 2014/05/27 13:27:03, perkj wrote:
> xians- want to take another look?
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> File content/renderer/media/media_stream_impl.h (right):
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> content/renderer/media/media_stream_impl.h:142: bool AreAllSourcesRemoved()
> const { return sources_.empty(); }
> On 2014/05/27 12:30:36, xians1 wrote:
> > change this name here to are_all_sources_removed() or simply has_no_source()
?
> If you don't mind I will not touch this more. I just removed the semicolon to
> pass lint.
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> File content/renderer/media/media_stream_renderer_factory.h (right):
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> content/renderer/media/media_stream_renderer_factory.h:40: }  // namespace
> content
> On 2014/05/27 12:30:36, xians1 wrote:
> > nit, add an empty line
> 
> Done.
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> File content/renderer/media/webmediaplayer_ms.h (right):
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/media/...
> content/renderer/media/webmediaplayer_ms.h:14: #include
> "content/renderer/media/media_stream_renderer_factory.h"
> On 2014/05/27 12:30:36, xians1 wrote:
> > why not forward the declaration?
> 
> Done.
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
> content/renderer/render_frame_impl.cc:3426: #if defined(ENABLE_WEBRTC)
> On 2014/05/27 12:30:36, xians1 wrote:
> > can you improve the #ifs here?
> > like 
> > 
> > #if !defined(ENABLE_WEBRTC)
> >   return NULL
> > #endif
> > 
> > #if defined(OS_ANDROID) && defined(ARCH_CPU_ARMEL)
> > ...
> > #endif
> > 
> > // WebRtc is enabled.
> > return new WebMediaPlayerMS(frame_, client, weak_factory_.GetWeakPtr(),
> 
> Possibly. But it means that the WebMediaPlayerMS must be compiled even if
WEBRTC
> is disabled. So I would like to not change this.
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/renderer/render...
> content/renderer/render_frame_impl.cc:3442:
> scoped_ptr<MediaStreamRendererFactory> factory(
> On 2014/05/27 12:30:36, xians1 wrote:
> > nit, simply do
> > return scoped_ptr<MediaStreamRendererFactory>(new
> MediaStreamRendererFactory());
> 
> Done.
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
> File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):
> 
>
https://codereview.chromium.org/294043015/diff/120001/content/shell/renderer/...
> content/shell/renderer/test_runner/web_frame_test_proxy.h:278: return
> factory.Pass();
> On 2014/05/27 12:30:36, xians1 wrote:
> > ditto
> 
> Done.

Lgtm

[perkj_chrome, 2014-05-27 17:23:23]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-27 17:23:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/294043015/140001

[commit-bot: I haz the power, 2014-05-27 20:11:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-27 23:47:11]

Change committed as 273073