Refactor the P2PSocketDispatcher to be created on the RenderThread instead of the RenderView.

Refactor the P2PSocketDispatcher to be created on the RenderThread instead of the RenderView.
Removes the use of routing_id. 

The reason for this refactoring is that we want to be able to create a PeerConnection on the RenderThread instead of the RenderView.
PeerConnection is dependent on the P2PSocketDispatcher.
PeerConnections are created in renderer_webkitplatformsupport_impl and should not be affilitated with a render view.
 https://chromiumcodereview.appspot.com/10703095/diff/13017/content/renderer/renderer_webkitplatformsupport_impl.cc

BUG=

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

[perkj_chrome, 2012-09-10 14:30:48]

Hi Sergey and Tommi. 
Would you mind taking a first glimpse on what it would mean to move the
P2PSocketDispatcher creation to the render thread?

This is seems to work with the current PeerConnection implementation. I will
update the unit test as a next step in this cl.

Sergey - is the routing_id not used for anything currently ? Ie if we do this-
there is no longer any association between a socket and a render view?

Thanks
Per

[perkj_chrome, 2012-09-10 15:55:50]

Oops - forgot to actually add Tommi.

[tommi (sloooow) - chröme, 2012-09-11 09:04:17]

This is great!  One request below.  I'll continue reviewing in a little while.

https://chromiumcodereview.appspot.com/10917167/diff/5001/content/browser/ren...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10917167/diff/5001/content/browser/ren...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:117:
IPC_BEGIN_MESSAGE_MAP_EX(P2PSocketDispatcherHost, message, *message_was_ok)
As discussed I think it would make the class a little simpler to use the non-EX
version of the message map here and not pass the |msg| to the message handlers. 
This also requires that we remove the routing id from the socket map which I
don't think we need anymore.

[tommi (sloooow) - chröme, 2012-09-11 12:09:09]

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:150: if (it ==
sockets_.end())
change this to:

return (it == sockets_.end()) ? NULL : it->second;

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:205:
sockets_.insert(SocketPair(socket_id, socket.release()));
without using SocketPair, this should just be:
sockets_[socket_id] = socket.release();

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:221:
sockets_.insert(SocketPair(connected_socket_id, accepted_connection));
sockets_[connected_socket_id] = accepted_connection;

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/browser/ren...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:43: typedef
std::pair<int, P2PSocketHost*> SocketPair;
You don't need this typedef actually.  See comments in the cc file.

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
File content/renderer/p2p/socket_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:32: delete msg;
maybe log an error or NOTREACHED?

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:36: loop->PostTask(
what if you're already on the right thread?  Can't you just send the message
directly in that case?

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:45: scoped_refptr<P2PSocketDispatcher>
message_sender_;
Since both AsyncMessageSender and P2PSocketDispatcher are now reference counted,
is there a way to guarantee that both get deleted?  If we have a circular
reference somehow, then these objects might be leaked.

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:87: delete message;
log warning/error?

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:110: // Captures the channel for IPC.
unnecessary comment

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.cc:119: channel_ = NULL;
I think we should use this opportunity to delete/release objects such as the
async_message_sender_.  Otherwise it looks to me that these objects will never
be deleted.

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
File content/renderer/p2p/socket_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:56: : public
IPC::ChannelProxy::MessageFilter{
space before {

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

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/re...
content/renderer/render_thread_impl.cc:313:
AddFilter(p2p_socket_dispatcher_.get());
nit: no .get()

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/re...
File content/renderer/render_thread_impl.h (right):

https://chromiumcodereview.appspot.com/10917167/diff/4005/content/renderer/re...
content/renderer/render_thread_impl.h:203: 
revert

[perkj_chrome, 2012-09-11 14:27:29]

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:150: if (it ==
sockets_.end())
On 2012/09/11 12:09:09, tommi wrote:
> change this to:
> 
> return (it == sockets_.end()) ? NULL : it->second;

Done.

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:205:
sockets_.insert(SocketPair(socket_id, socket.release()));
On 2012/09/11 12:09:09, tommi wrote:
> without using SocketPair, this should just be:
> sockets_[socket_id] = socket.release();

Done.

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:221:
sockets_.insert(SocketPair(connected_socket_id, accepted_connection));
On 2012/09/11 12:09:09, tommi wrote:
> sockets_[connected_socket_id] = accepted_connection;

Done.

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

http://codereview.chromium.org/10917167/diff/4005/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:43: typedef
std::pair<int, P2PSocketHost*> SocketPair;
On 2012/09/11 12:09:09, tommi wrote:
> You don't need this typedef actually.  See comments in the cc file.

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
File content/renderer/p2p/socket_dispatcher.cc (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:32: delete msg;
On 2012/09/11 12:09:09, tommi wrote:
> maybe log an error or NOTREACHED?

removed

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:36: loop->PostTask(
On 2012/09/11 12:09:09, tommi wrote:
> what if you're already on the right thread?  Can't you just send the message
> directly in that case?

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:45: scoped_refptr<P2PSocketDispatcher>
message_sender_;
Removed the AsyncMessageSender.
On 2012/09/11 12:09:09, tommi wrote:
> Since both AsyncMessageSender and P2PSocketDispatcher are now reference
counted,
> is there a way to guarantee that both get deleted?  If we have a circular
> reference somehow, then these objects might be leaked.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:87: delete message;
On 2012/09/11 12:09:09, tommi wrote:
> log warning/error?

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:110: // Captures the channel for IPC.
On 2012/09/11 12:09:09, tommi wrote:
> unnecessary comment

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:119: channel_ = NULL;
Removed it all together. 
Changed Send2PMessage to accept calls from other message loops.

On 2012/09/11 12:09:09, tommi wrote:
> I think we should use this opportunity to delete/release objects such as the
> async_message_sender_.  Otherwise it looks to me that these objects will never
> be deleted.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
File content/renderer/p2p/socket_dispatcher.h (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.h:56: : public
IPC::ChannelProxy::MessageFilter{
On 2012/09/11 12:09:09, tommi wrote:
> space before {

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:313:
AddFilter(p2p_socket_dispatcher_.get());
On 2012/09/11 12:09:09, tommi wrote:
> nit: no .get()

Done.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/render_thr...
content/renderer/render_thread_impl.h:203: 
On 2012/09/11 12:09:09, tommi wrote:
> revert

Done.

[perkj_chrome, 2012-09-11 14:34:26]

Adding Antoine.
Antoine, would you mind reviewing the files outside of p2p and media? 

Thanks

[tommi (sloooow) - chröme, 2012-09-11 15:08:58]

over all this looks pretty good but I'm a bit confused with some of the changes
in the last patch set. I'm guessing they're there due to a rebase?

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
File content/renderer/p2p/socket_dispatcher.cc (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:110: // Captures the channel for IPC.
On 2012/09/11 14:27:30, perkj wrote:
> On 2012/09/11 12:09:09, tommi wrote:
> > unnecessary comment
> 
> Done.

don't see the change

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:119: channel_ = NULL;
On 2012/09/11 14:27:30, perkj wrote:
> Removed it all together. 
> Changed Send2PMessage to accept calls from other message loops.
> 
> On 2012/09/11 12:09:09, tommi wrote:
> > I think we should use this opportunity to delete/release objects such as the
> > async_message_sender_.  Otherwise it looks to me that these objects will
never
> > be deleted.
> 

great!

[perkj_chrome, 2012-09-11 15:21:56]

Yes sorry.
New RenderView, RenderThread and MediaStreamImpl changes are due a rebase.

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
File content/renderer/p2p/socket_dispatcher.cc (right):

http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
content/renderer/p2p/socket_dispatcher.cc:110: // Captures the channel for IPC.
On 2012/09/11 15:08:58, tommi wrote:
> On 2012/09/11 14:27:30, perkj wrote:
> > On 2012/09/11 12:09:09, tommi wrote:
> > > unnecessary comment
> > 
> > Done.
> 
> don't see the change

Done.

[piman, 2012-09-11 16:11:27]

LGTM, nice refactoring!

[tommi (sloooow) - chröme, 2012-09-11 21:50:06]

On 2012/09/11 15:21:56, perkj wrote:
> Yes sorry.
> New RenderView, RenderThread and MediaStreamImpl changes are due a rebase.
> 
>
http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
> File content/renderer/p2p/socket_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/10917167/diff/4005/content/renderer/p2p/socket...
> content/renderer/p2p/socket_dispatcher.cc:110: // Captures the channel for
IPC.
> On 2012/09/11 15:08:58, tommi wrote:
> > On 2012/09/11 14:27:30, perkj wrote:
> > > On 2012/09/11 12:09:09, tommi wrote:
> > > > unnecessary comment
> > > 
> > > Done.
> > 
> > don't see the change
> 
> Done.

Great. LGTM.

[Sergey Ulanov, 2012-09-12 00:57:44]

LGTM with some style nits.

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:29:
net::HostResolver* host_resolver)
nit: now all parameters can fit on one line.

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:207:
sockets_[socket_id] =  socket.release();
insert() is a tad faster when you are adding new element to the map. insert()
also makes it cleaer that you are adding new element, not just assigning. If you
decide to keep assignment then remove the extra space after =.

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:223:
sockets_[connected_socket_id] =  accepted_connection;
same here.

http://codereview.chromium.org/10917167/diff/4035/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10917167/diff/4035/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:315: #endif
nit:  // defined(ENABLE_WEBRTC)

[perkj_chrome, 2012-09-12 07:40:59]

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:29:
net::HostResolver* host_resolver)
On 2012/09/12 00:57:44, sergeyu wrote:
> nit: now all parameters can fit on one line.

Done.

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:207:
sockets_[socket_id] =  socket.release();
Ok- I didn't now there are any performance differences. 
I will keep this anyway if you don't mind since the previous reviewer prefered
to get rid of the std::pair.

On 2012/09/12 00:57:44, sergeyu wrote:
> insert() is a tad faster when you are adding new element to the map. insert()
> also makes it cleaer that you are adding new element, not just assigning. If
you
> decide to keep assignment then remove the extra space after =.

http://codereview.chromium.org/10917167/diff/4035/content/browser/renderer_ho...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:223:
sockets_[connected_socket_id] =  accepted_connection;
dito
On 2012/09/12 00:57:44, sergeyu wrote:
> same here.

http://codereview.chromium.org/10917167/diff/4035/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10917167/diff/4035/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:315: #endif
On 2012/09/12 00:57:44, sergeyu wrote:
> nit:  // defined(ENABLE_WEBRTC)

Done.

[commit-bot: I haz the power, 2012-09-12 07:41:16]

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

[commit-bot: I haz the power, 2012-09-12 08:30:56]

Try job failure for 10917167-7014 (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...