WIP: websocket live experiment

WIP: websocket live experiment

BUG=none
TEST=none

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

[tyoshino (SeeGerritForStatus), 2009-10-27 10:20:29]

http://codereview.chromium.org/333045/diff/1/2
File chrome/browser/net/websocket_experiment_task.cc (right):

http://codereview.chromium.org/333045/diff/1/2#newcode61
Line 61: method_factory_.RevokeAll();
Comment "Revoke timeout timer" plz

http://codereview.chromium.org/333045/diff/1/2#newcode63
Line 63: if (next_state_ == STATE_URL_FETCH_COMPLETE && response_code == 200) {
Please return ERR_UNEXPECTED if the first condition is false.

http://codereview.chromium.org/333045/diff/1/2#newcode85
Line 85: if (!websocket_idle_start_time_.is_null()
&& on this line?

http://codereview.chromium.org/333045/diff/1/3
File chrome/browser/net/websocket_experiment_task.h (right):

http://codereview.chromium.org/333045/diff/1/3#newcode4
Line 4: //
Remove.

http://codereview.chromium.org/333045/diff/1/3#newcode17
Line 17: #include "net/base/completion_callback.h"
Can we forward declare CompletionCallback ?

http://codereview.chromium.org/333045/diff/1/3#newcode104
Line 104: 
Please explain when |callback| will be run.

[Yuzo, 2009-10-27 10:28:25]

http://codereview.chromium.org/333045/diff/1/2
File chrome/browser/net/websocket_experiment_task.cc (right):

http://codereview.chromium.org/333045/diff/1/2#newcode144
Line 144: case STATE_WEBSOCKET_IDLE:
How about naming this STATE_WEBSOCKET_WAIT?
To me, IDLE sounds like a passive and unintentional behavior.

http://codereview.chromium.org/333045/diff/1/2#newcode145
Line 145: result = DoWebSocketIdle();
Should we wait before receive?

To me, more natural scenario would be to wait before sending something, i.e.,
wait before sending HELLO or wait before sending back echo.

http://codereview.chromium.org/333045/diff/1/4
File chrome/chrome.gyp (right):

http://codereview.chromium.org/333045/diff/1/4#newcode1790
Line 1790: 'browser/net/websocket_experiment_task.h',
I'm not sure if it is preferred for Chromium, but
how about creating a new directory dedicated for this experiment?
For example, browser/net/experiment/websocket_experiment_task.{cc,h} ?

That way it would be easier to remove all the only-for-experiment files later
(and to avoid inadvertent use of classes/functions in the experimental files by
somebody else).

[ukai, 2009-10-28 06:59:13]

http://codereview.chromium.org/333045/diff/1/2
File chrome/browser/net/websocket_experiment_task.cc (right):

http://codereview.chromium.org/333045/diff/1/2#newcode61
Line 61: method_factory_.RevokeAll();
On 2009/10/27 10:20:29, tyoshino wrote:
> Comment "Revoke timeout timer" plz

Done.

http://codereview.chromium.org/333045/diff/1/2#newcode63
Line 63: if (next_state_ == STATE_URL_FETCH_COMPLETE && response_code == 200) {
On 2009/10/27 10:20:29, tyoshino wrote:
> Please return ERR_UNEXPECTED if the first condition is false.

Done.

http://codereview.chromium.org/333045/diff/1/2#newcode85
Line 85: if (!websocket_idle_start_time_.is_null()
On 2009/10/27 10:20:29, tyoshino wrote:
> && on this line?

Done.

http://codereview.chromium.org/333045/diff/1/2#newcode144
Line 144: case STATE_WEBSOCKET_IDLE:
On 2009/10/27 10:28:25, Yuzo wrote:
> How about naming this STATE_WEBSOCKET_WAIT?
> To me, IDLE sounds like a passive and unintentional behavior.

Hmm, it's actually not only waiting for something, but make sure it is idle (no
event should not be occured) for some specific time.

http://codereview.chromium.org/333045/diff/1/2#newcode145
Line 145: result = DoWebSocketIdle();
On 2009/10/27 10:28:25, Yuzo wrote:
> Should we wait before receive?

Yes. Keep connection idle while and see it it is not closed and server could
push some message.

> To me, more natural scenario would be to wait before sending something, i.e.,
> wait before sending HELLO or wait before sending back echo.

http://codereview.chromium.org/333045/diff/1/3
File chrome/browser/net/websocket_experiment_task.h (right):

http://codereview.chromium.org/333045/diff/1/3#newcode4
Line 4: //
On 2009/10/27 10:20:29, tyoshino wrote:
> Remove.

Done.

http://codereview.chromium.org/333045/diff/1/3#newcode17
Line 17: #include "net/base/completion_callback.h"
On 2009/10/27 10:20:29, tyoshino wrote:
> Can we forward declare CompletionCallback ?

No.
error: using typedef-name 'net::CompletionCallback' after 'class'

http://codereview.chromium.org/333045/diff/1/3#newcode104
Line 104: 
On 2009/10/27 10:20:29, tyoshino wrote:
> Please explain when |callback| will be run.

Done.

http://codereview.chromium.org/333045/diff/1/4
File chrome/chrome.gyp (right):

http://codereview.chromium.org/333045/diff/1/4#newcode1790
Line 1790: 'browser/net/websocket_experiment_task.h',
On 2009/10/27 10:28:25, Yuzo wrote:
> I'm not sure if it is preferred for Chromium, but
> how about creating a new directory dedicated for this experiment?
> For example, browser/net/experiment/websocket_experiment_task.{cc,h} ?
> 
> That way it would be easier to remove all the only-for-experiment files later
> (and to avoid inadvertent use of classes/functions in the experimental files
by
> somebody else).

Done.

[Yuzo, 2009-10-28 07:22:59]

Hmm, I only see update for .gyp, but not for .h and .cc.

Have you uploaded all the files? (or perhaps the review
system is not working properly now?)

[ukai, 2009-10-28 07:30:50]

On 2009/10/28 07:22:59, Yuzo wrote:
> Hmm, I only see update for .gyp, but not for .h and .cc.
> 
> Have you uploaded all the files? (or perhaps the review
> system is not working properly now?)

I think there were uploaded
(no diff since 1, because files were moved)

[tyoshino (SeeGerritForStatus), 2009-10-28 07:41:37]

LGTM

http://codereview.chromium.org/333045/diff/5001/5003
File chrome/browser/net/websocket_experiment/websocket_experiment_task.h
(right):

http://codereview.chromium.org/333045/diff/5001/5003#newcode8
Line 8: //  - Fetch |http_url_| within |url_fetch_deadline_ms| msec.
http_url

http://codereview.chromium.org/333045/diff/5001/5003#newcode11
Line 11: //  - Connect to |url_| with WebSocket protocol within
url_ -> url

http://codereview.chromium.org/333045/diff/5001/5003#newcode17
Line 17: //    Checks message can be sent/recevied on the WebSocket connection.
received

http://codereview.chromium.org/333045/diff/5001/5003#newcode22
Line 22: //  - Wait some message from server within
Wait for?

http://codereview.chromium.org/333045/diff/5001/5003#newcode26
Line 26: //  - Wait |websocket_bye_message| message from server within
within -> for? I'm not sure which is better.
Or
Expect that ... arrives within ... msec?

[Yuzo, 2009-10-28 07:44:35]

LGTM

[ukai, 2009-10-28 08:13:24]

http://codereview.chromium.org/333045/diff/5001/5003
File chrome/browser/net/websocket_experiment/websocket_experiment_task.h
(right):

http://codereview.chromium.org/333045/diff/5001/5003#newcode8
Line 8: //  - Fetch |http_url_| within |url_fetch_deadline_ms| msec.
On 2009/10/28 07:41:38, tyoshino wrote:
> http_url

Done.

http://codereview.chromium.org/333045/diff/5001/5003#newcode11
Line 11: //  - Connect to |url_| with WebSocket protocol within
On 2009/10/28 07:41:38, tyoshino wrote:
> url_ -> url

Done.

http://codereview.chromium.org/333045/diff/5001/5003#newcode17
Line 17: //    Checks message can be sent/recevied on the WebSocket connection.
On 2009/10/28 07:41:38, tyoshino wrote:
> received

Done.

http://codereview.chromium.org/333045/diff/5001/5003#newcode22
Line 22: //  - Wait some message from server within
On 2009/10/28 07:41:38, tyoshino wrote:
> Wait for?

Done.

http://codereview.chromium.org/333045/diff/5001/5003#newcode26
Line 26: //  - Wait |websocket_bye_message| message from server within
On 2009/10/28 07:41:38, tyoshino wrote:
> within -> for? I'm not sure which is better.
> Or
> Expect that ... arrives within ... msec?

Done.