Implement websocket throttling.

Implement websocket throttling.

Implement the client-side requirements in the spec.
 4.1 Handshake
   1.   If the user agent already has a Web Socket connection to the
        remote host (IP address) identified by /host/, even if known by
        another name, wait until that connection has been established or
        for that connection to have failed.

BUG=none
TEST=net_unittests passes

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

[tyoshino (SeeGerritForStatus), 2009-11-02 08:44:23]

http://codereview.chromium.org/342052/diff/3001/4002
File net/net.gyp (right):

http://codereview.chromium.org/342052/diff/3001/4002#newcode653
Line 653: 'websockets/websocket_throttler_unittest.cc',
sort lexicographically?

http://codereview.chromium.org/342052/diff/3001/4003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/3001/4003#newcode683
Line 683: DidReceiveData(result);
What about this? result = Did...?

http://codereview.chromium.org/342052/diff/3001/4003#newcode705
Line 705: DidSendData(result);
ditto

http://codereview.chromium.org/342052/diff/3001/4006
File net/socket_stream/socket_stream_throttler.h (right):

http://codereview.chromium.org/342052/diff/3001/4006#newcode37
Line 37: }
blank line please

http://codereview.chromium.org/342052/diff/3001/4007
File net/websockets/websocket_throttler.cc (right):

http://codereview.chromium.org/342052/diff/3001/4007#newcode115
Line 115: callback->Run(OK);
How about resuming asynchronously?

http://codereview.chromium.org/342052/diff/3001/4007#newcode148
Line 148: 
remove?

http://codereview.chromium.org/342052/diff/3001/4007#newcode237
Line 237: 
remove

http://codereview.chromium.org/342052/diff/3001/4008
File net/websockets/websocket_throttler.h (right):

http://codereview.chromium.org/342052/diff/3001/4008#newcode45
Line 45: // If other socket is using the same destination address, set |state|
is
s/is//

http://codereview.chromium.org/342052/diff/3001/4008#newcode54
Line 54: // every queue for the detination addresses of |socket|.
destination

[Yuzo, 2009-11-02 09:50:25]

s/throtting/throttling/ in the issue description.

I don't see any thread synchronization in this patch.
Is it OK? Is it guaranteed that only one thread touches a connection queue at
any moment?

http://codereview.chromium.org/342052/diff/3001/4001
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/342052/diff/3001/4001#newcode12
Line 12: #include "net/websockets/websocket_throttler.h"
s/throttler/throttle/
?
throttle is also a noun.

http://codereview.chromium.org/342052/diff/3001/4004
File net/socket_stream/socket_stream.h (right):

http://codereview.chromium.org/342052/diff/3001/4004#newcode196
Line 196: friend class WebSocketThrottlerTest;
Insert a new line here?

http://codereview.chromium.org/342052/diff/3001/4006
File net/socket_stream/socket_stream_throttler.h (right):

http://codereview.chromium.org/342052/diff/3001/4006#newcode25
Line 25: CompletionCallback* callback) {
How about adding TODO (or FIXME?) comment?

Ditto for the following currently-not-implemented methods.

http://codereview.chromium.org/342052/diff/3001/4007
File net/websockets/websocket_throttler.cc (right):

http://codereview.chromium.org/342052/diff/3001/4007#newcode264
Line 264: class WebSocketThrottlerInitSingleton {
Why do we need this class? To call
SocketStreamThrottler::RegisterSocketStreamThrottler only once for each scheme?

Because SocketStreamThrottlerRegistry  is a signleton,
can we just register throttlers in the constructor?

[ukai, 2009-11-04 05:45:09]

http://codereview.chromium.org/342052/diff/3001/4001
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/342052/diff/3001/4001#newcode12
Line 12: #include "net/websockets/websocket_throttler.h"
On 2009/11/02 09:50:26, Yuzo wrote:
> s/throttler/throttle/
> ?
> throttle is also a noun.

Done.

http://codereview.chromium.org/342052/diff/3001/4002
File net/net.gyp (right):

http://codereview.chromium.org/342052/diff/3001/4002#newcode653
Line 653: 'websockets/websocket_throttler_unittest.cc',
On 2009/11/02 08:44:23, tyoshino wrote:
> sort lexicographically?

Done.

http://codereview.chromium.org/342052/diff/3001/4003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/3001/4003#newcode683
Line 683: DidReceiveData(result);
On 2009/11/02 08:44:23, tyoshino wrote:
> What about this? result = Did...?

Done.

http://codereview.chromium.org/342052/diff/3001/4003#newcode705
Line 705: DidSendData(result);
On 2009/11/02 08:44:23, tyoshino wrote:
> ditto

Done.

http://codereview.chromium.org/342052/diff/3001/4004
File net/socket_stream/socket_stream.h (right):

http://codereview.chromium.org/342052/diff/3001/4004#newcode196
Line 196: friend class WebSocketThrottlerTest;
On 2009/11/02 09:50:26, Yuzo wrote:
> Insert a new line here?

Done.

http://codereview.chromium.org/342052/diff/3001/4006
File net/socket_stream/socket_stream_throttler.h (right):

http://codereview.chromium.org/342052/diff/3001/4006#newcode25
Line 25: CompletionCallback* callback) {
On 2009/11/02 09:50:26, Yuzo wrote:
> How about adding TODO (or FIXME?) comment?
> 
> Ditto for the following currently-not-implemented methods.

This is default implementation.
real implementation for websocket protocol is implemented in
net/websocket/websocket_throttle.cc

http://codereview.chromium.org/342052/diff/3001/4006#newcode37
Line 37: }
On 2009/11/02 08:44:23, tyoshino wrote:
> blank line please

Done.

http://codereview.chromium.org/342052/diff/3001/4007
File net/websockets/websocket_throttler.cc (right):

http://codereview.chromium.org/342052/diff/3001/4007#newcode115
Line 115: callback->Run(OK);
On 2009/11/02 08:44:23, tyoshino wrote:
> How about resuming asynchronously?

Done.

http://codereview.chromium.org/342052/diff/3001/4007#newcode148
Line 148: 
On 2009/11/02 08:44:23, tyoshino wrote:
> remove?

Done.

http://codereview.chromium.org/342052/diff/3001/4007#newcode237
Line 237: 
On 2009/11/02 08:44:23, tyoshino wrote:
> remove

Done.

http://codereview.chromium.org/342052/diff/3001/4007#newcode264
Line 264: class WebSocketThrottlerInitSingleton {
On 2009/11/02 09:50:26, Yuzo wrote:
> Why do we need this class? To call
> SocketStreamThrottler::RegisterSocketStreamThrottler only once for each
scheme?
> 
> Because SocketStreamThrottlerRegistry  is a signleton,
> can we just register throttlers in the constructor?
> 

Done.

http://codereview.chromium.org/342052/diff/3001/4008
File net/websockets/websocket_throttler.h (right):

http://codereview.chromium.org/342052/diff/3001/4008#newcode45
Line 45: // If other socket is using the same destination address, set |state|
is
On 2009/11/02 08:44:23, tyoshino wrote:
> s/is//

Done.

http://codereview.chromium.org/342052/diff/3001/4008#newcode54
Line 54: // every queue for the detination addresses of |socket|.
On 2009/11/02 08:44:23, tyoshino wrote:
> destination

Done.

[Yuzo, 2009-11-04 06:22:26]

http://codereview.chromium.org/342052/diff/5001/6003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/5001/6003#newcode256
Line 256: &io_callback_);
Extra space before &io_...

http://codereview.chromium.org/342052/diff/5001/6007
File net/websockets/websocket_throttle.cc (right):

http://codereview.chromium.org/342052/diff/5001/6007#newcode98
Line 98: memcpy(buffer_->data(), data, len);
These two lines should be outside of else{} clause?

[ukai, 2009-11-04 06:29:45]

http://codereview.chromium.org/342052/diff/5001/6003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/5001/6003#newcode256
Line 256: &io_callback_);
On 2009/11/04 06:22:26, Yuzo wrote:
> Extra space before &io_...

Done.

http://codereview.chromium.org/342052/diff/5001/6007
File net/websockets/websocket_throttle.cc (right):

http://codereview.chromium.org/342052/diff/5001/6007#newcode98
Line 98: memcpy(buffer_->data(), data, len);
On 2009/11/04 06:22:26, Yuzo wrote:
> These two lines should be outside of else{} clause?

Done.

[tyoshino (SeeGerritForStatus), 2009-11-04 07:07:41]

LGTM

http://codereview.chromium.org/342052/diff/5001/6003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/5001/6003#newcode244
Line 244: if (!delegate_)
nit: how about do either of the followings?
- do throttle and read_buf_ = NULL regardless delegate == NULL or not
- return immediately if delegate == NULL with do nothing

http://codereview.chromium.org/342052/diff/5001/6003#newcode258
Line 258: if (!delegate_)
nit: ditto

http://codereview.chromium.org/342052/diff/5001/6007
File net/websockets/websocket_throttle.cc (right):

http://codereview.chromium.org/342052/diff/5001/6007#newcode114
Line 114: callback_ = NULL;
Say that we wrap callback_ to prevent callback_ from being released.

http://codereview.chromium.org/342052/diff/5001/6007#newcode123
Line 123: class CallbackRunner : public
base::RefCountedThreadSafe<CallbackRunner> {
I know that this is in net:: namespace and private, but I prefer not using the
name "CallbackRunner" that exists in global namespace in base/task.h . How about
CompletionCallbackWrapper or something?

http://codereview.chromium.org/342052/diff/5001/6008
File net/websockets/websocket_throttle.h (right):

http://codereview.chromium.org/342052/diff/5001/6008#newcode58
Line 58: // Key: string of address.  Value: queue of sockets for the address.
hashed address?

[ukai, 2009-11-04 07:49:59]

http://codereview.chromium.org/342052/diff/5001/6003
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/342052/diff/5001/6003#newcode244
Line 244: if (!delegate_)
On 2009/11/04 07:07:41, tyoshino wrote:
> nit: how about do either of the followings?
> - do throttle and read_buf_ = NULL regardless delegate == NULL or not
> - return immediately if delegate == NULL with do nothing
> 

Done.

http://codereview.chromium.org/342052/diff/5001/6003#newcode258
Line 258: if (!delegate_)
On 2009/11/04 07:07:41, tyoshino wrote:
> nit: ditto

Done.

http://codereview.chromium.org/342052/diff/5001/6007
File net/websockets/websocket_throttle.cc (right):

http://codereview.chromium.org/342052/diff/5001/6007#newcode114
Line 114: callback_ = NULL;
On 2009/11/04 07:07:41, tyoshino wrote:
> Say that we wrap callback_ to prevent callback_ from being released.

Done.

http://codereview.chromium.org/342052/diff/5001/6007#newcode123
Line 123: class CallbackRunner : public
base::RefCountedThreadSafe<CallbackRunner> {
On 2009/11/04 07:07:41, tyoshino wrote:
> I know that this is in net:: namespace and private, but I prefer not using the
> name "CallbackRunner" that exists in global namespace in base/task.h . How
about
> CompletionCallbackWrapper or something?

Done.

http://codereview.chromium.org/342052/diff/5001/6008
File net/websockets/websocket_throttle.h (right):

http://codereview.chromium.org/342052/diff/5001/6008#newcode58
Line 58: // Key: string of address.  Value: queue of sockets for the address.
On 2009/11/04 07:07:41, tyoshino wrote:
> hashed address?

it's string of host's address.

[tyoshino (SeeGerritForStatus), 2009-11-04 08:11:08]

LGTM

[Yuzo, 2009-11-04 08:20:12]

LGTM except the following.

http://codereview.chromium.org/342052/diff/8002/9002
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/342052/diff/8002/9002#newcode16
Line 16: net::WebSocketThrottle::Init();
Why do we need this?
Do we have any reasons to require the existence of the singleton at this moment?

[ukai, 2009-11-04 08:29:20]

http://codereview.chromium.org/342052/diff/8002/9002
File chrome/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

http://codereview.chromium.org/342052/diff/8002/9002#newcode16
Line 16: net::WebSocketThrottle::Init();
On 2009/11/04 08:20:12, Yuzo wrote:
> Why do we need this?
> Do we have any reasons to require the existence of the singleton at this
moment?

We need to call Init to construct the singleton somewhere (it's not
automatically constructed), and I think it's good place to do it.  It should be
before the first SocketStream constructed (and I don't want to make SocketStream
depends on WebSocket.)