Move creation of PeerConnection from the RenderView to the RenderThreadImpl.

Move creation of PeerConnection from the RenderView to the RenderThreadImpl.

This is a proposed solution for the problem with using WebFrame* web_frame = WebFrame::frameForCurrentContext() in 
renderer_webkitplatformsupport_impl.cc

https://chromiumcodereview.appspot.com/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc

This should be seen as step 1 in refactoring how PeerConnections are created and used. If PeerConnections should be created in the RenderThreadImpl - so should all objects that it is dependent on such as the P2PSocketDispatcher that is currently created and owned by the RenderViewImpl. 


BUG=

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

[perkj_chrome, 2012-09-06 15:27:25]

Hi All. 
It make sence to me to move the creation of PeerConnection from the RenderView
to the RenderThreadImpl.
This cl moves the MediaStreamDependencyFactory to the RenderThreadImpl and make
use of it to create PeerConnection.
This way we don't need the WebFrame to find the correct RenderView.

Can you please help me review and say what you think.

Thanks
Per

[piman, 2012-09-06 17:04:45]

I think it's great. Just a couple of nits, and we need to resolve the lifetime
of the PeerConnectionFactory but other than that, it's going into the right
direction.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:24: //
media_stream_impl. Find a better way of doing this.
Can we just move it to a common header?

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:110: return
pc_handler;
style nit: if you just return new PeerConnectionHandlerJsep(client, this); I
think it fits in one line.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_impl.cc:433:
dependency_factory_->ReleasePeerConnectionFactory();
Should this logic move to RenderThreadImpl? IIUC you have a one MediaStreamImpl
per RenderView, but a single dependency factory per renderer. Won't this release
the peer connection factory for all RenderViews?
If it holds resources that are important to be released once all webrtc-using
tabs are gone, you could add a refcount. If it's just a small factory object, it
having the lifetime of the renderer isn't a problem.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
content/renderer/render_thread_impl.cc:1115: if (!media_stream_factory_.get()) {
nit: no need for braces.

[darin (slow to review), 2012-09-06 18:24:59]

I see, so the explicit dependency on RenderView can just be eliminated.  That's
great!  LGTM w/ nits fixed.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:20: namespace {
nit: add new line below here.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:23: // The enum and
UpdateWebRTCMethodCount must bee kept in sync with
bee -> be

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:26:
kWebkitGetUserMedia,
nit: chromium follows the old google c++ style rule that has enum values in
UPPER_CASE style.

http://dev.chromium.org/developers/coding-style#Naming

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:34: static void
UpdateWebRTCMethodCount(JavaScriptAPIName api_name) {
nit: "static" keyword inside anonymous namespace is redundant.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
content/renderer/render_thread_impl.cc:1120: 
nit: only one new line here

[tommi (sloooow) - chröme, 2012-09-07 11:23:51]

lgtm as well with all comments addressed.

[perkj_chrome, 2012-09-12 11:53:14]

Tommmi - can you help me review now? P2PSocketDispatcher is landed.

http://codereview.chromium.org/10919122/diff/2001/content/renderer/media/medi...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/10919122/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:20: namespace {
On 2012/09/06 18:24:59, darin wrote:
> nit: add new line below here.

Moved to webrtc_uma_histograms.

http://codereview.chromium.org/10919122/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:24: //
media_stream_impl. Find a better way of doing this.
On 2012/09/06 17:04:45, piman wrote:
> Can we just move it to a common header?

Done.

http://codereview.chromium.org/10919122/diff/2001/content/renderer/media/medi...
content/renderer/media/media_stream_dependency_factory.cc:34: static void
UpdateWebRTCMethodCount(JavaScriptAPIName api_name) {
On 2012/09/06 18:24:59, darin wrote:
> nit: "static" keyword inside anonymous namespace is redundant.

Done.

[tommi (sloooow) - chröme, 2012-09-12 13:10:24]

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_center.cc (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_center.cc:87:
MediaStreamCenter::MediaStreamCenter(WebKit::WebMediaStreamCenterClient*,
missing variable name

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_center.h (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_center.h:24:
MediaStreamCenter(WebKit::WebMediaStreamCenterClient*,
missing variable name

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:71:
content::P2PSocketDispatcher* socket_dispatcher_;
can this now be a scoped_refptr?

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:279: signaling_thread_
= jingle_glue::JingleThreadWrapper::current();
should we CHECK that this isn't NULL?

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:303:
&MediaStreamDependencyFactory::CreateIpcNetworkManagerOnWorkerThread,
fix indent

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:314: if
(!PeerConnectionFactoryCreated()) {
could you have this check at the top and return early if this condition is met?
(I'm assuming it does all the other checks anyway)

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.h (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.h:80: 
only one space

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.h:126:
scoped_refptr<content::P2PSocketDispatcher> p2p_socket_dispatcher_;
can we always fetch this from the render thread perhaps?

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:45: CHECK(devices.size() ==
webkit_sources.size());
CHECK_EQ?

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
File content/renderer/media/media_stream_source_extra_data.h (right):

http://codereview.chromium.org/10919122/diff/12001/content/renderer/media/med...
content/renderer/media/media_stream_source_extra_data.h:27:
media_stream::StreamDeviceInfo device_info_;
fix indent

http://codereview.chromium.org/10919122/diff/24/content/renderer/media/webrtc...
File content/renderer/media/webrtc_uma_histograms.h (right):

http://codereview.chromium.org/10919122/diff/24/content/renderer/media/webrtc...
content/renderer/media/webrtc_uma_histograms.h:14: kWebkitGetUserMedia,
follow chrome style

[perkj_chrome, 2012-09-12 13:54:38]

Fixed issues found by Tommi.
Antoine - can you take another look?

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:23: // The enum and
UpdateWebRTCMethodCount must bee kept in sync with
On 2012/09/06 18:24:59, darin wrote:
> bee -> be

Done.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:26:
kWebkitGetUserMedia,
On 2012/09/06 18:24:59, darin wrote:
> nit: chromium follows the old google c++ style rule that has enum values in
> UPPER_CASE style.
> 
> http://dev.chromium.org/developers/coding-style#Naming

Done.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:110: return
pc_handler;
On 2012/09/06 17:04:45, piman wrote:
> style nit: if you just return new PeerConnectionHandlerJsep(client, this); I
> think it fits in one line.

Done.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/me...
content/renderer/media/media_stream_impl.cc:433:
dependency_factory_->ReleasePeerConnectionFactory();
On 2012/09/06 17:04:45, piman wrote:
> Should this logic move to RenderThreadImpl? IIUC you have a one
MediaStreamImpl
> per RenderView, but a single dependency factory per renderer. Won't this
release
> the peer connection factory for all RenderViews?
> If it holds resources that are important to be released once all webrtc-using
> tabs are gone, you could add a refcount. If it's just a small factory object,
it
> having the lifetime of the renderer isn't a problem.

This have not moved to the dependency factory and there is only one per render
thread. CleanupPeerConnectionFactory is called as part of the
MediaStreamDependencyFactory dtor.

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
content/renderer/render_thread_impl.cc:1115: if (!media_stream_factory_.get()) {
On 2012/09/06 17:04:45, piman wrote:
> nit: no need for braces.

now its needed....

https://chromiumcodereview.appspot.com/10919122/diff/2001/content/renderer/re...
content/renderer/render_thread_impl.cc:1120: 
On 2012/09/06 18:24:59, darin wrote:
> nit: only one new line here

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_center.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_center.cc:87:
MediaStreamCenter::MediaStreamCenter(WebKit::WebMediaStreamCenterClient*,
On 2012/09/12 13:10:24, tommi wrote:
> missing variable name

Added but it is unused.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_center.h (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_center.h:24:
MediaStreamCenter(WebKit::WebMediaStreamCenterClient*,
On 2012/09/12 13:10:24, tommi wrote:
> missing variable name

Added - but it is unused.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:71:
content::P2PSocketDispatcher* socket_dispatcher_;
On 2012/09/12 13:10:24, tommi wrote:
> can this now be a scoped_refptr?

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:279: signaling_thread_
= jingle_glue::JingleThreadWrapper::current();
On 2012/09/12 13:10:24, tommi wrote:
> should we CHECK that this isn't NULL?

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:303:
&MediaStreamDependencyFactory::CreateIpcNetworkManagerOnWorkerThread,
On 2012/09/12 13:10:24, tommi wrote:
> fix indent

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:314: if
(!PeerConnectionFactoryCreated()) {
On 2012/09/12 13:10:24, tommi wrote:
> could you have this check at the top and return early if this condition is
met?
> (I'm assuming it does all the other checks anyway)

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.h (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.h:80: 
On 2012/09/12 13:10:24, tommi wrote:
> only one space

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.h:126:
scoped_refptr<content::P2PSocketDispatcher> p2p_socket_dispatcher_;
On 2012/09/12 13:10:24, tommi wrote:
> can we always fetch this from the render thread perhaps?
It would be hard to test using the mock_dependency_factory in that case.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_impl.cc:45: CHECK(devices.size() ==
webkit_sources.size());
On 2012/09/12 13:10:24, tommi wrote:
> CHECK_EQ?

Done.

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
File content/renderer/media/media_stream_source_extra_data.h (right):

https://chromiumcodereview.appspot.com/10919122/diff/12001/content/renderer/m...
content/renderer/media/media_stream_source_extra_data.h:27:
media_stream::StreamDeviceInfo device_info_;
On 2012/09/12 13:10:24, tommi wrote:
> fix indent

Done.

https://chromiumcodereview.appspot.com/10919122/diff/24/content/renderer/medi...
File content/renderer/media/webrtc_uma_histograms.h (right):

https://chromiumcodereview.appspot.com/10919122/diff/24/content/renderer/medi...
content/renderer/media/webrtc_uma_histograms.h:14: kWebkitGetUserMedia,
On 2012/09/12 13:10:24, tommi wrote:
> follow chrome style

Done.

[tommi (sloooow) - chröme, 2012-09-12 14:13:06]

lgtm

[piman, 2012-09-12 17:09:20]

lgtm

[commit-bot: I haz the power, 2012-09-12 18:17:17]

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

[commit-bot: I haz the power, 2012-09-12 18:32:23]

Try job failure for 10919122-9020 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-12 20:59:01]

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

[commit-bot: I haz the power, 2012-09-12 21:35:40]

Try job failure for 10919122-9020 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-12 23:32:52]

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

[commit-bot: I haz the power, 2012-09-13 00:20:29]

Try job failure for 10919122-9020 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-13 02:09:05]

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

[commit-bot: I haz the power, 2012-09-13 02:43:24]

Try job failure for 10919122-9020 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...