Refactor SocketStream to report error to the delegate.

Refactor SocketStream to report error to the delegate.

error status will be reported to the delegate (e.g. websocket experiment task)

BUG=none
TEST=none

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

[Yuzo, 2009-10-29 08:57:28]

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

http://codereview.chromium.org/346010/diff/1/2#newcode291
Line 291: else
DCHECK(result == OK) ?
Or result can be other than OK here?

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

http://codereview.chromium.org/346010/diff/1/3#newcode83
Line 83: // Called when some error occured.
s/some/an/ ?

http://codereview.chromium.org/346010/diff/1/3#newcode84
Line 84: // The delegate doesn't need to call Close(). SocketStream may close
What methods can the delegate call? I guess the delegate should not call any
methods that change SocketStream's state. 

So this method is only for error logging? Or can the delegate
try to recover from the error anyhow?

http://codereview.chromium.org/346010/diff/1/3#newcode85
Line 85: // after some error occurs.
Except for proxy resolution error?

http://codereview.chromium.org/346010/diff/1/3#newcode194
Line 194: // Finish the job.
s/Finish/Finishes/

http://codereview.chromium.org/346010/diff/1/3#newcode195
Line 195: // If |result| is not net::OK, calls OnError of delegate.
Hmm, as far as I can tell from the code, OnError is called
also when result==net::OK if the state is STATE_NONE.

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

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

http://codereview.chromium.org/346010/diff/1/2#newcode193
Line 193: delegate_->OnError(this, result);
indentation

http://codereview.chromium.org/346010/diff/1/2#newcode351
Line 351: } while (result != ERR_IO_PENDING && next_state_ != STATE_NONE);
How about removing "next_state_ != STATE_NONE" and replacing L353, L354 with
DCHECK(next_state_ != STATE_NONE)?

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

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

http://codereview.chromium.org/346010/diff/1/2#newcode193
Line 193: delegate_->OnError(this, result);
On 2009/10/29 09:08:20, tyoshino wrote:
> indentation

Done.

http://codereview.chromium.org/346010/diff/1/2#newcode291
Line 291: else
On 2009/10/29 08:57:28, Yuzo wrote:
> DCHECK(result == OK) ?
> Or result can be other than OK here?

Done.

http://codereview.chromium.org/346010/diff/1/2#newcode351
Line 351: } while (result != ERR_IO_PENDING && next_state_ != STATE_NONE);
On 2009/10/29 09:08:20, tyoshino wrote:
> How about removing "next_state_ != STATE_NONE" and replacing L353, L354 with
> DCHECK(next_state_ != STATE_NONE)?

Done.

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

http://codereview.chromium.org/346010/diff/1/3#newcode83
Line 83: // Called when some error occured.
On 2009/10/29 08:57:28, Yuzo wrote:
> s/some/an/ ?

Done.

http://codereview.chromium.org/346010/diff/1/3#newcode84
Line 84: // The delegate doesn't need to call Close(). SocketStream may close
On 2009/10/29 08:57:28, Yuzo wrote:
> What methods can the delegate call? I guess the delegate should not call any
> methods that change SocketStream's state. 
> 
> So this method is only for error logging? Or can the delegate
> try to recover from the error anyhow?

This method is only for error reporting. Update the comment.

http://codereview.chromium.org/346010/diff/1/3#newcode194
Line 194: // Finish the job.
On 2009/10/29 08:57:28, Yuzo wrote:
> s/Finish/Finishes/

Done.

http://codereview.chromium.org/346010/diff/1/3#newcode195
Line 195: // If |result| is not net::OK, calls OnError of delegate.
On 2009/10/29 08:57:28, Yuzo wrote:
> Hmm, as far as I can tell from the code, OnError is called
> also when result==net::OK if the state is STATE_NONE.

which line?
Actually, Finish() may not be called with net::OK. Fixed the comment.

[Yuzo, 2009-10-29 10:34:03]

LGTM

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

http://codereview.chromium.org/346010/diff/1/3#newcode195
Line 195: // If |result| is not net::OK, calls OnError of delegate.
On 2009/10/29 10:22:30, ukai wrote:
> On 2009/10/29 08:57:28, Yuzo wrote:
> > Hmm, as far as I can tell from the code, OnError is called
> > also when result==net::OK if the state is STATE_NONE.
> 
> which line?
> Actually, Finish() may not be called with net::OK. Fixed the comment.
> 

Sorry, I was confused. Please ignore the comment.

[tyoshino (SeeGerritForStatus), 2009-10-30 01:49:38]

LGTM