SSLServerSocketTest.DataTransfer should keep reading until it has read

SSLServerSocketTest.DataTransfer should keep reading until it has read
the expected number of bytes.

We cannot assume a single SSL read is enough to read all the bytes.
This depends on how the SSL sender fragments the data into SSL records,
and whether the SSL receiver returns all available records at once or
one record at a time.

R=hclam@chromium.org,rvargas@chromium.org
BUG=none
TEST=net_unittests --gtest_filter=SSLServerSocketTest.DataTransfer

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

[wtc, 2011-08-13 01:50:21]

hclam: you are the primary reviewer because you wrote the
SSLServerSocketTest.DataTransfer unit test in
http://codereview.chromium.org/5746003

rvargas: please review the use of DrainableIOBuffer.

The motivation for this CL is http://codereview.chromium.org/7621002/
which changes how the SSL sender fragments the data into
SSL records.

Note: it is unfortunate that I have to repeat the reading
code -- one standalone read, and one read loop.  The
standalone read is necessary to preserve the "write then read"
and "read then write" tests.

[Alpha Left Google, 2011-08-15 10:30:28]

LGTM. Though I think this "reading from async socket until done" code is used in
many place, maybe we can put this as a util function.

[rvargas (doing something else), 2011-08-15 18:31:41]

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:384: client_ret =
client_socket_->Read(read_buf, read_buf->BytesRemaining(),
Is it important for this test that the write and the read start before running
the message loop? If not, we could get the result of the write and then loop for
the read instead of making a first read outside of the loop.

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:388: if (server_ret ==
net::ERR_IO_PENDING) {
This can be simplified with write_callback.GetResult().

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:404: EXPECT_GT(client_ret, 0);
If we really care about failures, we should add code to handle them because
right now we could get stuck in the loop.

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:406: bytes_read += client_ret;
nit: we could use BytesConsumed() here, instead of keeping an external counter.

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:413: // Read then write.
Looks like we are not really doing this.

[wtc, 2011-08-16 00:04:05]

rvargas: thank you for your comments.  I made all the changes
you suggested.  Please review Patch Set 2.

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/7648010/diff/1/net/socket/ssl_server_socket_un...
net/socket/ssl_server_socket_unittest.cc:384: client_ret =
client_socket_->Read(read_buf, read_buf->BytesRemaining(),
On 2011/08/15 18:31:41, rvargas wrote:
> Is it important for this test that the write and the read start before running
> the message loop? If not, we could get the result of the write and then loop
for
> the read instead of making a first read outside of the loop.

I am not sure either.  I can implement this change in
Patch Set 3 and let hclam pick one.

[wtc, 2011-08-16 00:11:13]

hclam: please review the diffs between Patch Sets 2 and 3,
and let me know if the simpler Patch Set 3 is acceptable to
you.  Thanks.

[rvargas (doing something else), 2011-08-16 00:18:45]

LGTM

[commit-bot: I haz the power, 2011-08-17 04:59:24]

Change committed as 97098