WebSocket support in chromium.

WebSocket support in chromium.

Run with --enable-web-sockets enables WebSocket features.

BUG=12497
TEST=none

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

[tyoshino (SeeGerritForStatus), 2009-10-22 03:34:18]

http://codereview.chromium.org/292044/diff/1/7
File chrome/browser/renderer_host/socket_stream_dispatcher_host.h (right):

http://codereview.chromium.org/292044/diff/1/7#newcode21
Line 21: public:
indentation.

http://codereview.chromium.org/292044/diff/1/9
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/1/9#newcode24
Line 24: public:
indentation

http://codereview.chromium.org/292044/diff/1/16
File chrome/renderer/socket_stream_dispatcher.h (right):

http://codereview.chromium.org/292044/diff/1/16#newcode20
Line 20: public:
indentation

[tyoshino (SeeGerritForStatus), 2009-10-22 04:32:17]

http://codereview.chromium.org/292044/diff/1/6
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/1/6#newcode48
Line 48: 
Could you add here some horizontal line and say the following methods are
SocketStream::Delegate implementation since the method names are similar to ones
of IPC handlers.

http://codereview.chromium.org/292044/diff/1/6#newcode52
Line 52: LOG(INFO) << "SocketStreamDispatcherHost::OnConnected socket_id="
LOG(INFO) is too loud? I'm not sure.

ditto for followings

http://codereview.chromium.org/292044/diff/1/6#newcode67
Line 67: LOG(INFO) << "SocketStreamDispatcherHost::OnDataSent socket_id="
s/OnDataSent/OnSentData/

http://codereview.chromium.org/292044/diff/1/6#newcode73
Line 73: new ViewMsg_SocketStream_SentData(socket_id, amount_sent))) {
indentation?

http://codereview.chromium.org/292044/diff/1/6#newcode74
Line 74: LOG(ERROR) << "ViewMsg_SocketStream_DataSent failed.";
s/DataSent/SentData/

http://codereview.chromium.org/292044/diff/1/6#newcode101
Line 101: 
ditto. horizontal line here please.

http://codereview.chromium.org/292044/diff/1/6#newcode120
Line 120: return;
any log or something on debug mode? or LOG(ERROR)?

http://codereview.chromium.org/292044/diff/1/6#newcode128
Line 128: LOG(INFO) << "SocketStreamDispatcherHost::OnClose socket_id=" <<
socket_id;
s/OnClose/OnCloseReq/

[tyoshino (SeeGerritForStatus), 2009-10-22 04:40:56]

http://codereview.chromium.org/292044/diff/1/10
File chrome/chrome.gyp (right):

http://codereview.chromium.org/292044/diff/1/10#newcode558
Line 558: 'common/net/socket_stream.h',
Is .cc unnecessary?

http://codereview.chromium.org/292044/diff/1/12
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/292044/diff/1/12#newcode154
Line 154: db_message_filter_ = NULL;
If we have to reset this explicitly here, please add comment to explain that
here.

http://codereview.chromium.org/292044/diff/1/14
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/292044/diff/1/14#newcode253
Line 253: RenderThread::current()->socket_stream_dispatcher();
indentation?

[ukai, 2009-10-22 05:23:58]

http://codereview.chromium.org/292044/diff/1/6
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/1/6#newcode48
Line 48: 
On 2009/10/22 04:32:17, tyoshino wrote:
> Could you add here some horizontal line and say the following methods are
> SocketStream::Delegate implementation since the method names are similar to
ones
> of IPC handlers.

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode52
Line 52: LOG(INFO) << "SocketStreamDispatcherHost::OnConnected socket_id="
On 2009/10/22 04:32:17, tyoshino wrote:
> LOG(INFO) is too loud? I'm not sure.
> 
> ditto for followings

Hmm, make them DLOG(INFO)

http://codereview.chromium.org/292044/diff/1/6#newcode67
Line 67: LOG(INFO) << "SocketStreamDispatcherHost::OnDataSent socket_id="
On 2009/10/22 04:32:17, tyoshino wrote:
> s/OnDataSent/OnSentData/

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode73
Line 73: new ViewMsg_SocketStream_SentData(socket_id, amount_sent))) {
On 2009/10/22 04:32:17, tyoshino wrote:
> indentation?

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode74
Line 74: LOG(ERROR) << "ViewMsg_SocketStream_DataSent failed.";
On 2009/10/22 04:32:17, tyoshino wrote:
> s/DataSent/SentData/

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode101
Line 101: 
On 2009/10/22 04:32:17, tyoshino wrote:
> ditto. horizontal line here please.

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode120
Line 120: return;
On 2009/10/22 04:32:17, tyoshino wrote:
> any log or something on debug mode? or LOG(ERROR)?

Done.

http://codereview.chromium.org/292044/diff/1/6#newcode128
Line 128: LOG(INFO) << "SocketStreamDispatcherHost::OnClose socket_id=" <<
socket_id;
On 2009/10/22 04:32:17, tyoshino wrote:
> s/OnClose/OnCloseReq/

Done.

http://codereview.chromium.org/292044/diff/1/7
File chrome/browser/renderer_host/socket_stream_dispatcher_host.h (right):

http://codereview.chromium.org/292044/diff/1/7#newcode21
Line 21: public:
On 2009/10/22 03:34:18, tyoshino wrote:
> indentation.

Done.

http://codereview.chromium.org/292044/diff/1/9
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/1/9#newcode24
Line 24: public:
On 2009/10/22 03:34:18, tyoshino wrote:
> indentation

Done.

http://codereview.chromium.org/292044/diff/1/10
File chrome/chrome.gyp (right):

http://codereview.chromium.org/292044/diff/1/10#newcode558
Line 558: 'common/net/socket_stream.h',
On 2009/10/22 04:40:56, tyoshino wrote:
> Is .cc unnecessary?

no.

http://codereview.chromium.org/292044/diff/1/12
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/292044/diff/1/12#newcode154
Line 154: db_message_filter_ = NULL;
On 2009/10/22 04:40:56, tyoshino wrote:
> If we have to reset this explicitly here, please add comment to explain that
> here.

Done.

http://codereview.chromium.org/292044/diff/1/14
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/292044/diff/1/14#newcode253
Line 253: RenderThread::current()->socket_stream_dispatcher();
On 2009/10/22 04:40:56, tyoshino wrote:
> indentation?

Done.

http://codereview.chromium.org/292044/diff/1/16
File chrome/renderer/socket_stream_dispatcher.h (right):

http://codereview.chromium.org/292044/diff/1/16#newcode20
Line 20: public:
On 2009/10/22 03:34:18, tyoshino wrote:
> indentation

Done.

[tyoshino (SeeGerritForStatus), 2009-10-22 05:50:48]

http://codereview.chromium.org/292044/diff/1/15
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/1/15#newcode18
Line 18: // It communites with the main browser process via
SocketStreamDispatcher.
s/communites/communicates/

http://codereview.chromium.org/292044/diff/1/15#newcode159
Line 159: return new IPCWebSocketStreamHandleBridge(
indentation

[ukai, 2009-10-22 05:53:25]

http://codereview.chromium.org/292044/diff/1/15
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/1/15#newcode18
Line 18: // It communites with the main browser process via
SocketStreamDispatcher.
On 2009/10/22 05:50:48, tyoshino wrote:
> s/communites/communicates/

Done.

http://codereview.chromium.org/292044/diff/1/15#newcode159
Line 159: return new IPCWebSocketStreamHandleBridge(
On 2009/10/22 05:50:48, tyoshino wrote:
> indentation

Done.

[Yuzo, 2009-10-22 06:01:55]

http://codereview.chromium.org/292044/diff/1/6
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/1/6#newcode17
Line 17: SocketStreamDispatcherHost::~SocketStreamDispatcherHost() {
Two questions:
Q1: Do we really need to remove elements in hosts_?
    If I read the code correctly, SocketStreamHost instances
    are not deleted by merely removing them from hosts_.
    (Call DeleteSocketStreamHost?)
Q2: If we do need to remove all elements, how about adding a method
    RemoveAll (or so) to IDMap and call it from here?

http://codereview.chromium.org/292044/diff/1/6#newcode98
Line 98: return;
No need to notify the renderer of the closure?

http://codereview.chromium.org/292044/diff/1/9
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/1/9#newcode29
Line 29: // Get socket_id associated with |socket|.
s/Get/Gets/

Ditto for some others below.

http://codereview.chromium.org/292044/diff/1/9#newcode30
Line 30: static int SocketIdFromSocketStream(net::SocketStream* socket);
Are there any SockStream instances that don't have socket ID?

How about make all SockStream instances have socket ID and socket_id() method?

http://codereview.chromium.org/292044/diff/7001/5011
File chrome/common/net/socket_stream.h (right):

http://codereview.chromium.org/292044/diff/7001/5011#newcode10
Line 10: static const int kNoSocketId = 0;
Do we really need 'static' here?

[Yuzo, 2009-10-22 06:43:03]

Additional comments.

http://codereview.chromium.org/292044/diff/2003/2016
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/292044/diff/2003/2016#newcode252
Line 252: SocketStreamDispatcher* dispatch =
s/dispatch/dispatcher/
?
2 characters out of 10 aren't worth saving, are they?

http://codereview.chromium.org/292044/diff/2003/2017
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/2003/2017#newcode2
Line 2: // Author: ukai@google.com (Fumitoshi Ukai)
// Copyright (c) 2009 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

http://codereview.chromium.org/292044/diff/2003/2017#newcode145
Line 145: LOG(INFO) << "Connect failed?";
Why '?' ?

http://codereview.chromium.org/292044/diff/2003/2017#newcode146
Line 146: DCHECK_EQ(socket_id_, chrome_common_net::kNoSocketId);
Will this hold? Where can socket_id_ be changed after line 138: socket_id_ =
all_bridges.Add(this); ?

http://codereview.chromium.org/292044/diff/2003/2017#newcode182
Line 182: if (bridge)
Can bridge evaluate to false in normal execution?
(Perhaps the connection is closed before OnConnected is
called??)

Is it worth logging (or even DCHECK?) ?

http://codereview.chromium.org/292044/diff/2003/2018
File chrome/renderer/socket_stream_dispatcher.h (right):

http://codereview.chromium.org/292044/diff/2003/2018#newcode18
Line 18: // creates an instance and delegates calls to it.
s/instance/instance of SocketStreamDispatcher/
?

[ukai, 2009-10-22 07:07:42]

http://codereview.chromium.org/292044/diff/1/6
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/1/6#newcode17
Line 17: SocketStreamDispatcherHost::~SocketStreamDispatcherHost() {
On 2009/10/22 06:01:55, Yuzo wrote:
> Two questions:
> Q1: Do we really need to remove elements in hosts_?
>     If I read the code correctly, SocketStreamHost instances
>     are not deleted by merely removing them from hosts_.
>     (Call DeleteSocketStreamHost?)

Ah, it must delete SocketStreamHost instances.

> Q2: If we do need to remove all elements, how about adding a method
>     RemoveAll (or so) to IDMap and call it from here?
Added it as TODO.

http://codereview.chromium.org/292044/diff/1/6#newcode98
Line 98: return;
On 2009/10/22 06:01:55, Yuzo wrote:
> No need to notify the renderer of the closure?

if socket_id is kNoSocketId, it would be wrong situation.
otherwise, DeleteSocketStreamHost will notify renderer.

http://codereview.chromium.org/292044/diff/1/9
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/1/9#newcode29
Line 29: // Get socket_id associated with |socket|.
On 2009/10/22 06:01:55, Yuzo wrote:
> s/Get/Gets/
> 
> Ditto for some others below.

Done.

http://codereview.chromium.org/292044/diff/1/9#newcode30
Line 30: static int SocketIdFromSocketStream(net::SocketStream* socket);
On 2009/10/22 06:01:55, Yuzo wrote:
> Are there any SockStream instances that don't have socket ID?
> 
> How about make all SockStream instances have socket ID and socket_id() method?

socket_id is used for SocketStreamHost only.
Other client of SocketStream (in test_shell or in net/websockets) doesn't use
socket_id.

http://codereview.chromium.org/292044/diff/7001/5011
File chrome/common/net/socket_stream.h (right):

http://codereview.chromium.org/292044/diff/7001/5011#newcode10
Line 10: static const int kNoSocketId = 0;
On 2009/10/22 06:01:56, Yuzo wrote:
> Do we really need 'static' here?

Done.

http://codereview.chromium.org/292044/diff/2003/2016
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/292044/diff/2003/2016#newcode252
Line 252: SocketStreamDispatcher* dispatch =
On 2009/10/22 06:43:03, Yuzo wrote:
> s/dispatch/dispatcher/
> ?
> 2 characters out of 10 aren't worth saving, are they?

Done.
I just followed the style of ResourceDispatcher above.

http://codereview.chromium.org/292044/diff/2003/2017
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/2003/2017#newcode2
Line 2: // Author: ukai@google.com (Fumitoshi Ukai)
On 2009/10/22 06:43:03, Yuzo wrote:
> // Copyright (c) 2009 The Chromium Authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.

Done.

http://codereview.chromium.org/292044/diff/2003/2017#newcode145
Line 145: LOG(INFO) << "Connect failed?";
On 2009/10/22 06:43:03, Yuzo wrote:
> Why '?' ?

Fixed the message.

http://codereview.chromium.org/292044/diff/2003/2017#newcode146
Line 146: DCHECK_EQ(socket_id_, chrome_common_net::kNoSocketId);
On 2009/10/22 06:43:03, Yuzo wrote:
> Will this hold? Where can socket_id_ be changed after line 138: socket_id_ =
> all_bridges.Add(this); ?

Ah, it's old code. Removed.

http://codereview.chromium.org/292044/diff/2003/2017#newcode182
Line 182: if (bridge)
On 2009/10/22 06:43:03, Yuzo wrote:
> Can bridge evaluate to false in normal execution?
> (Perhaps the connection is closed before OnConnected is
> called??)
> 
> Is it worth logging (or even DCHECK?) ?

Done.

http://codereview.chromium.org/292044/diff/2003/2018
File chrome/renderer/socket_stream_dispatcher.h (right):

http://codereview.chromium.org/292044/diff/2003/2018#newcode18
Line 18: // creates an instance and delegates calls to it.
On 2009/10/22 06:43:03, Yuzo wrote:
> s/instance/instance of SocketStreamDispatcher/
> ?

Done.

[Yuzo, 2009-10-22 07:18:47]

LGTM

http://codereview.chromium.org/292044/diff/7004/7018
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/7004/7018#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
Should be "Chromium Authors"?

[ukai, 2009-10-22 08:10:30]

http://codereview.chromium.org/292044/diff/7004/7018
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/7004/7018#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
On 2009/10/22 07:18:47, Yuzo wrote:
> Should be "Chromium Authors"?

Done.

[tyoshino (SeeGerritForStatus), 2009-10-22 09:07:58]

http://codereview.chromium.org/292044/diff/4005/3024
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/4005/3024#newcode83
Line 83: DeleteSocketStreamHost(socket_id);
DeleteSocket... is better than socket_stream_host->Close()?
Could you educate me the reason how you chose one of them for L83, L99, L140?

http://codereview.chromium.org/292044/diff/4005/3024#newcode97
Line 97: socket_id, std::vector<char>(data, data + len)))) {
indentation?

http://codereview.chromium.org/292044/diff/4005/3027
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/4005/3027#newcode16
Line 16: // Each SocketStramHandle will have an unique socket_id assigned by
s/Stram/Stream/

http://codereview.chromium.org/292044/diff/4005/3033
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/4005/3033#newcode88
Line 88: LOG(INFO) << "Send data.size=" << data.size();
DLOG?

http://codereview.chromium.org/292044/diff/4005/3033#newcode164
Line 164: LOG(INFO) << "SocketStreamDispatcher::OnMessageReceived";
DLOG?

[ukai, 2009-10-22 09:54:54]

http://codereview.chromium.org/292044/diff/4005/3024
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/292044/diff/4005/3024#newcode83
Line 83: DeleteSocketStreamHost(socket_id);
On 2009/10/22 09:07:58, tyoshino wrote:
> DeleteSocket... is better than socket_stream_host->Close()?
> Could you educate me the reason how you chose one of them for L83, L99, L140?

DeleteSocketStreamHost is used to delete SocketStream when sending msg from
browser to renderer.
socket_stream_host->Close() is used when handling msg from renderer to browser.

http://codereview.chromium.org/292044/diff/4005/3024#newcode97
Line 97: socket_id, std::vector<char>(data, data + len)))) {
On 2009/10/22 09:07:58, tyoshino wrote:
> indentation?

Done.

http://codereview.chromium.org/292044/diff/4005/3027
File chrome/browser/renderer_host/socket_stream_host.h (right):

http://codereview.chromium.org/292044/diff/4005/3027#newcode16
Line 16: // Each SocketStramHandle will have an unique socket_id assigned by
On 2009/10/22 09:07:58, tyoshino wrote:
> s/Stram/Stream/

Done.

http://codereview.chromium.org/292044/diff/4005/3033
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/4005/3033#newcode88
Line 88: LOG(INFO) << "Send data.size=" << data.size();
On 2009/10/22 09:07:58, tyoshino wrote:
> DLOG?

Done.

http://codereview.chromium.org/292044/diff/4005/3033#newcode164
Line 164: LOG(INFO) << "SocketStreamDispatcher::OnMessageReceived";
On 2009/10/22 09:07:58, tyoshino wrote:
> DLOG?

Done.

[tyoshino (SeeGerritForStatus), 2009-10-23 01:48:15]

LGTM

[Denis Lagno, 2011-01-27 12:21:12]

http://codereview.chromium.org/292044/diff/13016/chrome/renderer/socket_strea...
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/13016/chrome/renderer/socket_strea...
chrome/renderer/socket_stream_dispatcher.cc:147: OnClosed();
is not it an error?  You call OnClosed() which performs Release() but no
AddRef() was executed on this branch?

[ukai, 2011-01-28 05:31:28]

http://codereview.chromium.org/292044/diff/13016/chrome/renderer/socket_strea...
File chrome/renderer/socket_stream_dispatcher.cc (right):

http://codereview.chromium.org/292044/diff/13016/chrome/renderer/socket_strea...
chrome/renderer/socket_stream_dispatcher.cc:147: OnClosed();
On 2011/01/27 12:21:13, Denis Lagno wrote:
> is not it an error?  You call OnClosed() which performs Release() but no
> AddRef() was executed on this branch?

Ah. good catch!
I'll fix it.