P2P sockets IPC dispatcher for the renderer side.

P2P sockets IPC dispatcher for the renderer side.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76731

[awong, 2011-03-01 20:21:55]

some comments.  Mostly based on style and comments.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
File chrome/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:69: void
P2PSocketClient::OnSocketCreated(P2PSocketAddress address) {
DCHECK the thread, here and below?

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:81: const std::vector<char>& data) {
spacing...maybe run cpplint on these files?

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:82: DCHECK_EQ(state_, STATE_OPEN);
use the expected state as the first argument here and elsewhere.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
File chrome/renderer/p2p/socket_client.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:16: // P2P socket that rountes all calls
over . Can be created
Comment is unfinished.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:31: ~P2PSocketClient();
Should the destructor be private or protected?

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:45: int id() const { return id_; }
What is this id?  Add comment?

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
File chrome/renderer/p2p/sockets_dispatcher.cc (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:72: int socket_id, P2PSocketAddress
address) {
We we care what threads these are called on?  Should we DCHECK the messge loop?

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:99: // hasn't processed the close
message by the time it send the
send -> sends

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:101: LOG(INFO) << "Received
ViewMsg_P2P_OnDataReceived for socket that "
This sounds more like a VLOG than an LOG_INFO).

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
File chrome/renderer/p2p/sockets_dispatcher.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:7: 
We need a comment showing how these classes interact.  Similar to the one inside
chrome/renderer/media/audio_renderer_impl.h

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:15: class P2PSocketsDispatcher : public
IPC::ChannelProxy::MessageFilter {
Aren't these usually named XXXXFilter?  So P2pSocketsFilter?

[Alpha Left Google, 2011-03-01 21:32:20]

It looks about right. However I have the following concerns:

1. The use of refcounting for SocketClient
2. Thread semantics, please add comments about how and where these objects are
used
3. Does a plugin has a correponding RenderView? It is not clear to me how this
is used by the plugin, some pointers to code or CL would be nice.

http://codereview.chromium.org/6602039/diff/20/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/6602039/diff/20/chrome/chrome_renderer.gypi#ne...
chrome/chrome_renderer.gypi:135: 'renderer/p2p/sockets_dispatcher.cc',
hm.. it should be all singular, look at other files.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
File chrome/renderer/p2p/socket_client.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:18: class P2PSocketClient : public
base::RefCountedThreadSafe<P2PSocketClient> {
Why is this refcounted? Should this be only used on the IO thread? Please look
at ResourceDispatcher, it's not refcounted too.

Please add comments on how this class should be used so we can decide the
threading semantics.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:49: STATE_UNINITIALIZED,
nit: use kStateXXX.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
File chrome/renderer/p2p/sockets_dispatcher.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:15: class P2PSocketsDispatcher : public
IPC::ChannelProxy::MessageFilter {
Looks like this lives on the IO thread, please comment.

[Alpha Left Google, 2011-03-01 21:32:48]

Also one last thing:

4. Please show me the definition of the IPC messages and make sure they are
*always* non-blocking.

[Sergey Ulanov, 2011-03-01 22:42:55]

On 2011/03/01 21:32:48, Alpha wrote:
> Also one last thing:
> 
> 4. Please show me the definition of the IPC messages and make sure they are
> *always* non-blocking.
Here is pending CL for the other end of the connection that defines IPC messages
http://codereview.chromium.org/6598053/ (I've update messages names and file
locations, hasn't update this CL yet, will do it tomorrow).
All messages are asynchronous. 
I'll address your other comments tomorrow.

[Sergey Ulanov, 2011-03-02 16:12:28]

http://codereview.chromium.org/6602039/diff/20/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

http://codereview.chromium.org/6602039/diff/20/chrome/chrome_renderer.gypi#ne...
chrome/chrome_renderer.gypi:135: 'renderer/p2p/sockets_dispatcher.cc',
On 2011/03/01 21:32:20, Alpha wrote:
> hm.. it should be all singular, look at other files.
Done

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
File chrome/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:69: void
P2PSocketClient::OnSocketCreated(P2PSocketAddress address) {
On 2011/03/01 20:21:55, awong wrote:
> DCHECK the thread, here and below?

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:81: const std::vector<char>& data) {
On 2011/03/01 20:21:55, awong wrote:
> spacing...maybe run cpplint on these files?

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.cc:82: DCHECK_EQ(state_, STATE_OPEN);
On 2011/03/01 20:21:55, awong wrote:
> use the expected state as the first argument here and elsewhere.
Why? Most most of remoting code has expected value on the right, and I find this
more readable.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
File chrome/renderer/p2p/socket_client.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:16: // P2P socket that rountes all calls
over . Can be created
On 2011/03/01 20:21:55, awong wrote:
> Comment is unfinished.

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:18: class P2PSocketClient : public
base::RefCountedThreadSafe<P2PSocketClient> {
On 2011/03/01 21:32:20, Alpha wrote:
> Why is this refcounted? Should this be only used on the IO thread? 
It can be used from any thread. It will be used in PacketSocketAllocator that
will be called from chromoting jingle thread. It wouldn't need to be refcounted
if it was used only from IO thread.

> Please look
> at ResourceDispatcher, it's not refcounted too.
> 
> Please add comments on how this class should be used so we can decide the
> threading semantics.
Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:31: ~P2PSocketClient();
On 2011/03/01 20:21:55, awong wrote:
> Should the destructor be private or protected?

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:45: int id() const { return id_; }
On 2011/03/01 20:21:55, awong wrote:
> What is this id?  Add comment?
it is id of the socket. Renamed it to socket_id().

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/socket_cli...
chrome/renderer/p2p/socket_client.h:49: STATE_UNINITIALIZED,
On 2011/03/01 21:32:20, Alpha wrote:
> nit: use kStateXXX.
http://www.chromium.org/developers/coding-style says that MACRO_STYLE should be
used for enums.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
File chrome/renderer/p2p/sockets_dispatcher.cc (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:72: int socket_id, P2PSocketAddress
address) {
On 2011/03/01 20:21:55, awong wrote:
> We we care what threads these are called on?  Should we DCHECK the messge
loop?
Added DCHECK

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:99: // hasn't processed the close
message by the time it send the
On 2011/03/01 20:21:55, awong wrote:
> send -> sends

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.cc:101: LOG(INFO) << "Received
ViewMsg_P2P_OnDataReceived for socket that "
On 2011/03/01 20:21:55, awong wrote:
> This sounds more like a VLOG than an LOG_INFO).

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
File chrome/renderer/p2p/sockets_dispatcher.h (right):

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:7: 
On 2011/03/01 20:21:55, awong wrote:
> We need a comment showing how these classes interact.  Similar to the one
inside
> chrome/renderer/media/audio_renderer_impl.h

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:15: class P2PSocketsDispatcher : public
IPC::ChannelProxy::MessageFilter {
On 2011/03/01 21:32:20, Alpha wrote:
> Looks like this lives on the IO thread, please comment.

Done.

http://codereview.chromium.org/6602039/diff/20/chrome/renderer/p2p/sockets_di...
chrome/renderer/p2p/sockets_dispatcher.h:15: class P2PSocketsDispatcher : public
IPC::ChannelProxy::MessageFilter {
On 2011/03/01 20:21:55, awong wrote:
> Aren't these usually named XXXXFilter?  So P2pSocketsFilter?
There are many that are named Dispatcher. I think that Dispatcher is more
descriptive name for this object. I also changed it to inherit from
RenderViewObserver, which makes Filter name even less attractive.

[Sergey Ulanov, 2011-03-02 20:58:05]

+jam
John, can you please make sure I integrate with RenderView correctly.

[Alpha Left Google, 2011-03-02 21:56:10]

Looks good to me if we get the name of classes right.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_c...
File chrome/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_c...
chrome/renderer/p2p/socket_client.cc:23: dispatcher_->message_loop()->PostTask(
In chromium code it's transitioning to use MessageLoopProxy when posting to
another thread, please consider doing so here.

Since in remoting code we control all the code and we know the teardown
sequence, but in the renderer I'd try to play safe.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
File chrome/renderer/p2p/socket_dispatcher.h (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
chrome/renderer/p2p/socket_dispatcher.h:16: //      P2PSocketsHost  <---------> 
P2PSocketDispatcher
Hm.. Having P2PSocketsHost and P2PSocketHost is confusing... Chromium convention
is to use

P2PSocketDispatcherHost and P2PSocketService, does it sound good to you?

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
chrome/renderer/p2p/socket_dispatcher.h:31: // P2PSocketDispatcher works on the
same thread it was created on. It
I'd just say it works only on render thread or IO thread. I assume we won't use
it anywhere else.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:81: #include
"chrome/renderer/p2p/socket_dispatcher.h"
nit: p2p should be above plugin

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view....
chrome/renderer/render_view.h:1365: P2PSocketDispatcher*
p2p_sockets_dispatcher_;
nit: p2p_socket_dispatcher_

[Sergey Ulanov, 2011-03-02 23:56:43]

I'd prefer to rename them in a separate CL later. Addressed all other comments.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_c...
File chrome/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_c...
chrome/renderer/p2p/socket_client.cc:23: dispatcher_->message_loop()->PostTask(
On 2011/03/02 21:56:10, Alpha wrote:
> In chromium code it's transitioning to use MessageLoopProxy when posting to
> another thread, please consider doing so here.
> 
> Since in remoting code we control all the code and we know the teardown
> sequence, but in the renderer I'd try to play safe. 

Fixed. Also added Detach() method here for the case when dispatcher is destroyed
before clients.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
File chrome/renderer/p2p/socket_dispatcher.h (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
chrome/renderer/p2p/socket_dispatcher.h:16: //      P2PSocketsHost  <---------> 
P2PSocketDispatcher
On 2011/03/02 21:56:10, Alpha wrote:
> Hm.. Having P2PSocketsHost and P2PSocketHost is confusing... Chromium
convention
> is to use
> 
> P2PSocketDispatcherHost and P2PSocketService, does it sound good to you?

I agree that the naming sucks. Thanks for your suggestions. Can I fix it in a
separate change?

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/p2p/socket_d...
chrome/renderer/p2p/socket_dispatcher.h:31: // P2PSocketDispatcher works on the
same thread it was created on. It
On 2011/03/02 21:56:10, Alpha wrote:
> I'd just say it works only on render thread or IO thread. I assume we won't
use
> it anywhere else.

Done.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:81: #include
"chrome/renderer/p2p/socket_dispatcher.h"
On 2011/03/02 21:56:10, Alpha wrote:
> nit: p2p should be above plugin

Done.

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6602039/diff/7001/chrome/renderer/render_view....
chrome/renderer/render_view.h:1365: P2PSocketDispatcher*
p2p_sockets_dispatcher_;
On 2011/03/02 21:56:10, Alpha wrote:
> nit: p2p_socket_dispatcher_

Done.

[Alpha Left Google, 2011-03-02 23:58:42]

looking good. LGTM.