Refactor WebSocket throttling feature.

Refactor WebSocket throttling feature.

Protocol specific handling should be done in SocketStreamJob subclasss,
so websocket throttling should be handled in WebSocketJob.

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

[tyoshino (SeeGerritForStatus), 2010-03-10 06:49:14]

http://codereview.chromium.org/669157/diff/1/3
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/669157/diff/1/3#newcode231
net/socket_stream/socket_stream.cc:231: if (delegate) {
Is there any case where detaching delegate precedes here? In such case,
connecting queues might be left not removed.

http://codereview.chromium.org/669157/diff/1/8
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/669157/diff/1/8#newcode21
net/websockets/websocket_job.h:21: // Chome security policy doesn't allow
renderer process (except dev tools)
Chrome

http://codereview.chromium.org/669157/diff/1/10
File net/websockets/websocket_throttle.h (right):

http://codereview.chromium.org/669157/diff/1/10#newcode43
net/websockets/websocket_throttle.h:43: 
just one blank line

[tyoshino (SeeGerritForStatus), 2010-03-10 07:32:31]

http://codereview.chromium.org/669157/diff/1/11
File net/websockets/websocket_throttle_unittest.cc (right):

http://codereview.chromium.org/669157/diff/1/11#newcode71
net/websockets/websocket_throttle_unittest.cc:71: 
Could you add here some simple diagram to show how jobs are queued in this test.

like...

Address | <-head  tail->
1.2.3.4 | w1 w2    w4
1.2.3.5 | w1    w3
1.2.3.6 | w1       w4 w5

http://codereview.chromium.org/669157/diff/1/11#newcode167
net/websockets/websocket_throttle_unittest.cc:167: 
Please add some comment to mention that this test item is for checking that
removal from connection queue is correctly done in case the connection is still
pending for some preceding request but is closed.

[ukai, 2010-03-10 09:35:09]

http://codereview.chromium.org/669157/diff/1/3
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/669157/diff/1/3#newcode231
net/socket_stream/socket_stream.cc:231: if (delegate) {
On 2010/03/10 06:49:14, tyoshino wrote:
> Is there any case where detaching delegate precedes here? In such case,
> connecting queues might be left not removed.

Ah. 
I added code in WebSocketJob::DetachDelegate() to remove job from connecting
queue.

http://codereview.chromium.org/669157/diff/1/8
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/669157/diff/1/8#newcode21
net/websockets/websocket_job.h:21: // Chome security policy doesn't allow
renderer process (except dev tools)
On 2010/03/10 06:49:14, tyoshino wrote:
> Chrome

Done.

http://codereview.chromium.org/669157/diff/1/10
File net/websockets/websocket_throttle.h (right):

http://codereview.chromium.org/669157/diff/1/10#newcode43
net/websockets/websocket_throttle.h:43: 
On 2010/03/10 06:49:14, tyoshino wrote:
> just one blank line

Done.

http://codereview.chromium.org/669157/diff/1/11
File net/websockets/websocket_throttle_unittest.cc (right):

http://codereview.chromium.org/669157/diff/1/11#newcode71
net/websockets/websocket_throttle_unittest.cc:71: 
On 2010/03/10 07:32:32, tyoshino wrote:
> Could you add here some simple diagram to show how jobs are queued in this
test.
> 
> like...
> 
> Address | <-head  tail->
> 1.2.3.4 | w1 w2    w4
> 1.2.3.5 | w1    w3
> 1.2.3.6 | w1       w4 w5
> 

Done.

http://codereview.chromium.org/669157/diff/1/11#newcode167
net/websockets/websocket_throttle_unittest.cc:167: 
On 2010/03/10 07:32:32, tyoshino wrote:
> Please add some comment to mention that this test item is for checking that
> removal from connection queue is correctly done in case the connection is
still
> pending for some preceding request but is closed.

Done.

[tyoshino (SeeGerritForStatus), 2010-03-12 05:54:12]

LGTM

http://codereview.chromium.org/669157/diff/8001/9006
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/669157/diff/8001/9006#newcode159
net/websockets/websocket_job.cc:159:
Singleton<WebSocketThrottle>::get()->WakeupSocketIfNecessary();
Please add a test case for checking this code works.

[ukai, 2010-03-12 07:16:18]

Thanks for review.
Added test and fixed some other test issues.

- we should return OK in DidReceiveData() and DidSendData()
  in this method, user callback will be called and may change state to
STATE_CLOSE. after DidReceiveData() or DidSendData(), it will call DoLoop() and
it checks DCHECK_LE(result, OK), but if we return original result, result would
be more than zero.  we don't need original result (read length or written
length) after calling user callback, and it's ok just to return OK.

- add enable_throttle_ in WebSocketJob to prevent failure in WebSocketJobTest. 
in this test, addresses_ in SocketStream is not constructed, so it will fail
when accessing addresses from RemoveFromQueue()
(maybe, we might fix AddressList::head() return NULL if data_.get() == NULL.)

http://codereview.chromium.org/669157/diff/8001/9006
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/669157/diff/8001/9006#newcode159
net/websockets/websocket_job.cc:159:
Singleton<WebSocketThrottle>::get()->WakeupSocketIfNecessary();
On 2010/03/12 05:54:13, tyoshino wrote:
> Please add a test case for checking this code works.

Done.

[ukai, 2010-03-15 09:24:07]

I changed lots to pass unittests.
Could you take a look again, please?
thanks

[tyoshino (SeeGerritForStatus), 2010-03-17 05:57:58]

LGTM

http://codereview.chromium.org/669157/diff/31001/32009
File net/websockets/websocket_job_unittest.cc (right):

http://codereview.chromium.org/669157/diff/31001/32009#newcode215
net/websockets/websocket_job_unittest.cc:215: addr.ai_family =- AF_INET;
negative?

http://codereview.chromium.org/669157/diff/31001/32009#newcode220
net/websockets/websocket_job_unittest.cc:220: addr.ai_addr =
reinterpret_cast<sockaddr*>(&addr);
&sa_in ?

http://codereview.chromium.org/669157/diff/31001/32012
File net/websockets/websocket_throttle_unittest.cc (right):

http://codereview.chromium.org/669157/diff/31001/32012#newcode166
net/websockets/websocket_throttle_unittest.cc:166: // 1.2.3.6 | w1       w4  w5
one space between w4 and w5?

http://codereview.chromium.org/669157/diff/31001/32012#newcode184
net/websockets/websocket_throttle_unittest.cc:184: // 1.2.3.6 | w1       w4  w5
w6
ditto

[ukai, 2010-03-17 07:07:16]

http://codereview.chromium.org/669157/diff/31001/32009
File net/websockets/websocket_job_unittest.cc (right):

http://codereview.chromium.org/669157/diff/31001/32009#newcode215
net/websockets/websocket_job_unittest.cc:215: addr.ai_family =- AF_INET;
On 2010/03/17 05:57:58, tyoshino wrote:
> negative?

Oops. fixed

http://codereview.chromium.org/669157/diff/31001/32009#newcode220
net/websockets/websocket_job_unittest.cc:220: addr.ai_addr =
reinterpret_cast<sockaddr*>(&addr);
On 2010/03/17 05:57:58, tyoshino wrote:
> &sa_in ?

Oops. fixed

http://codereview.chromium.org/669157/diff/31001/32012
File net/websockets/websocket_throttle_unittest.cc (right):

http://codereview.chromium.org/669157/diff/31001/32012#newcode166
net/websockets/websocket_throttle_unittest.cc:166: // 1.2.3.6 | w1       w4  w5
On 2010/03/17 05:57:58, tyoshino wrote:
> one space between w4 and w5?

Done.

http://codereview.chromium.org/669157/diff/31001/32012#newcode184
net/websockets/websocket_throttle_unittest.cc:184: // 1.2.3.6 | w1       w4  w5
w6
On 2010/03/17 05:57:58, tyoshino wrote:
> ditto

Done.