Add a boolean data member called_socket_read_ to help...

Add a boolean data member reading_body_from_socket_ to help
DoReadBodyComplete() determine whether a result of 0
actually comes from a socket read, indicating EOF.

In unit tests, need to call RunAllPending() before calling
idle_socket_count() to get an accurate idle socket count
because idle sockets are added to the connection pool with a
PostTask.

R=eroman
BUG=9880
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13511

[eroman, 2009-04-08 23:24:39]

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

http://codereview.chromium.org/63139/diff/1/2#newcode847
Line 847: called_socket_read_ = false;
I'm not enthusiastic about this extra variable.

Since we already know the single case where DoReadBody() will return 0, how
about just changing unfiltered_eof to duplicate that logic:

bool unfiltered_eof = (result == 0 && response_body_length_ != -1 &&
response_body_read_ >= response_body_length_);

http://codereview.chromium.org/63139/diff/1/4
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/63139/diff/1/4#newcode2250
Line 2250: EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
I wander if  for consistency, we should simply precede all calls to
idle_socket_count() with RunAllPending().

(I guess it is unecessary here, since callback.WaitForResult() is already
spinning the loop?).

http://codereview.chromium.org/63139/diff/1/4#newcode2881
Line 2881: trans->response_.was_cached = true;  // (Wouldn't ever actually be
true...)
my bad, thanks :)

[wtc, 2009-04-08 23:40:13]

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

http://codereview.chromium.org/63139/diff/1/2#newcode847
Line 847: called_socket_read_ = false;
On 2009/04/08 23:24:40, eroman wrote:
> I'm not enthusiastic about this extra variable.
> 
> Since we already know the single case where DoReadBody() will return 0, how
> about just changing unfiltered_eof to duplicate that logic:
> 
> bool unfiltered_eof = (result == 0 && response_body_length_ != -1 &&
> response_body_read_ >= response_body_length_);

Are you sure you want to repeat the code?  It takes time
to re-evaluate the boolean expression, and if we ever
change it, we'll need to remember to update the other
two copies.

Can I rename called_socket_read_ to make it more appealing?
How about read_from_socket_ or bypassed_read_?

http://codereview.chromium.org/63139/diff/1/4
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/63139/diff/1/4#newcode2250
Line 2250: EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
On 2009/04/08 23:24:40, eroman wrote:
> I wander if  for consistency, we should simply precede all calls to
> idle_socket_count() with RunAllPending().
> 
> (I guess it is unecessary here, since callback.WaitForResult() is already
> spinning the loop?).

Yes, RunAllPeding() is unnecessary here, but the reason
is that we know we only return a socket to the connection
pool in DoReadBodyComplete and the destructor, so we know
the idle_socket_count() cannot change before the
ReadTransaction() call.

[eroman, 2009-04-09 00:17:51]

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

http://codereview.chromium.org/63139/diff/1/2#newcode847
Line 847: called_socket_read_ = false;
On 2009/04/08 23:40:13, wtc wrote:
> On 2009/04/08 23:24:40, eroman wrote:
> > I'm not enthusiastic about this extra variable.
> > 
> > Since we already know the single case where DoReadBody() will return 0, how
> > about just changing unfiltered_eof to duplicate that logic:
> > 
> > bool unfiltered_eof = (result == 0 && response_body_length_ != -1 &&
> > response_body_read_ >= response_body_length_);
> 
> Are you sure you want to repeat the code?  It takes time
> to re-evaluate the boolean expression, and if we ever
> change it, we'll need to remember to update the other
> two copies.
> 
> Can I rename called_socket_read_ to make it more appealing?
> How about read_from_socket_ or bypassed_read_?
> 

If you decide to keep the bool, I slightly prefer your other name option, 
|bypassed_read_|. Your call.

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

http://codereview.chromium.org/63139/diff/1/3#newcode242
Line 242: // True if a Read call actually called connection_.socket()->Read() to
read
"a Read call" --> "the last Read call" ?

http://codereview.chromium.org/63139/diff/1/3#newcode244
Line 244: // reading from the socket.
Maybe mention that the variable only meaning between the states
STATE_READ_BODY/STATE_DRAIN_BODY_FOR_AUTH and
STATE_READ_BODY_COMPLETE/STATE_DRAIN_BODY_FOR_AUTH_COMPLETE ?

[eroman, 2009-04-09 00:21:30]

lgtm with previous comments.

[wtc, 2009-04-09 22:13:53]

OK, as we discussed, I renamed the boolean member
reading_body_from_socket_, and added a lot of comments.

[eroman, 2009-04-09 22:38:49]

LGTM.

Make sure to update the CL description before committing (to reflect new
variable name).

http://codereview.chromium.org/63139/diff/2001/3002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/63139/diff/2001/3002#newcode244
Line 244: // Boolean state variable:
This seems like a repetition of the variable type.