net: Rework UploadDataStream API by introducing Read().

Replace buf()+buf_len()+MarkConsumedAndFillBuffer()+GetBufferSize() with
a single function Read(). This is done by externalizing the read buffer.

As a byproduct, use of memmove() in handling of SPDY HTTP requests is
significantly reduced. There, a write is done with kMaxSpdyFrameChunkSize
which is about 2.8KB, at a time, that caused MarkConsumedAndFillBuffer()
to move the remaining 13.2KB data in the internal buffer to the beginning by
memmove(), which ends up memmoving 13.2KB of data every time a chunk of 2.8KB
is written.

Along the way, UploadDataStream::IsOnLastChunk() is removed, which is
no longer necessary. Some TODO(satish)s have also been addressed.

This is in preparation for adding asynchronous API to UploadDataStream.
This is why Read() takes IOBuffer*, rather than char*.

BUG=72001
TEST=try bots

Review URL: https://chromiumcodereview.appspot.com/9317055

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@120946 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 58 insertions, 19 deletions

diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index ff96716..bf6dfce 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -211,10 +211,16 @@ int SpdyHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
 
   CHECK(!request_body_stream_.get());
   if (request_body) {
-    if (request_body->size() || request_body->is_chunked())
+    if (request_body->size() || request_body->is_chunked()) {
       request_body_stream_.reset(request_body);
-    else
+      // Use kMaxSpdyFrameChunkSize as the buffer size, since the request
+      // body data is written with this size at a time.
+      raw_request_body_buf_ = new IOBufferWithSize(kMaxSpdyFrameChunkSize);
+      // The request body buffer is empty at first.
+      request_body_buf_ = new DrainableIOBuffer(raw_request_body_buf_, 0);
+    } else {
       delete request_body;
+    }
   }
 
   CHECK(!callback.is_null());
@@ -276,31 +282,50 @@ bool SpdyHttpStream::OnSendHeadersComplete(int status) {
 int SpdyHttpStream::OnSendBody() {
   CHECK(request_body_stream_.get());
 
-  int buf_len = static_cast<int>(request_body_stream_->buf_len());
-  if (!buf_len)
+  // TODO(satorux): Clean up the logic here. This behavior is weird. Reading
+  // of upload data should happen in OnSendBody(). crbug.com/113107.
+  //
+  // Nothing to send. This happens when OnSendBody() is first called.
+  // A read of the upload data stream is initiated in OnSendBodyComplete().
+  if (request_body_buf_->BytesRemaining() == 0)
     return OK;
-  bool is_chunked = request_body_stream_->is_chunked();
-  // TODO(satish): For non-chunked POST data, we set DATA_FLAG_FIN for all
-  // blocks of data written out. This is wrong if the POST data was larger than
-  // UploadDataStream::kBufSize as that is the largest buffer that
-  // UploadDataStream returns at a time and we'll be setting the FIN flag for
-  // each block of data written out.
-  bool eof = !is_chunked || request_body_stream_->IsOnLastChunk();
+
+  const bool eof = request_body_stream_->IsEOF();
   return stream_->WriteStreamData(
-      request_body_stream_->buf(), buf_len,
+      request_body_buf_,
+      request_body_buf_->BytesRemaining(),
       eof ? spdy::DATA_FLAG_FIN : spdy::DATA_FLAG_NONE);
 }
 
 int SpdyHttpStream::OnSendBodyComplete(int status, bool* eof) {
+  // |status| is the number of bytes written to the SPDY stream.
   CHECK(request_body_stream_.get());
+  *eof = false;
+
+  if (status > 0) {
+    request_body_buf_->DidConsume(status);
+    if (request_body_buf_->BytesRemaining()) {
+      // Go back to OnSendBody() to send the remaining data.
+      return OK;
+    }
+  }
+
+  // Check if the entire body data has been sent.
+  *eof = (request_body_stream_->IsEOF() &&
+          !request_body_buf_->BytesRemaining());
+  if (*eof)
+    return OK;
 
-  request_body_stream_->MarkConsumedAndFillBuffer(status);
-  *eof = request_body_stream_->eof();
-  if (!*eof &&
-      request_body_stream_->is_chunked() &&
-      !request_body_stream_->buf_len())
+  // Read the data from the request body stream.
+  const int bytes_read = request_body_stream_->Read(
+      raw_request_body_buf_, raw_request_body_buf_->size());
+  if (request_body_stream_->is_chunked() && bytes_read == ERR_IO_PENDING)
     return ERR_IO_PENDING;
+  // ERR_IO_PENDING with chunked encoding is the only possible error.
+  DCHECK_GE(bytes_read, 0);
 
+  request_body_buf_ = new DrainableIOBuffer(raw_request_body_buf_,
+                                            bytes_read);
   return OK;
 }
 
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index f8612af..2c087ed 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -22,6 +22,7 @@
 
 namespace net {
 
+class DrainableIOBuffer;
 class HttpResponseInfo;
 class IOBuffer;
 class SpdySession;
@@ -125,6 +126,11 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate,
   scoped_refptr<IOBuffer> user_buffer_;
   int user_buffer_len_;
 
+  // Temporary buffer used to read the request body from UploadDataStream.
+  scoped_refptr<IOBufferWithSize> raw_request_body_buf_;
+  // Wraps raw_request_body_buf_ to read the remaining data progressively.
+  scoped_refptr<DrainableIOBuffer> request_body_buf_;
+
   // Is there a scheduled read callback pending.
   bool buffered_read_callback_pending_;
   // Has more data been received from the network during the wait for the
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index ea4c397..47b4f6a 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -2226,7 +2226,12 @@ TEST_P(SpdyNetworkTransactionTest, FlowControlStallResume) {
   ASSERT_TRUE(stream != NULL);
   ASSERT_TRUE(stream->stream() != NULL);
   EXPECT_EQ(0, stream->stream()->send_window_size());
-  EXPECT_FALSE(stream->request_body_stream_->eof());
+  // All the body data should have been read.
+  // TODO(satorux): This is because of the weirdness in reading the request
+  // body in OnSendBodyComplete(). See crbug.com/113107.
+  EXPECT_TRUE(stream->request_body_stream_->IsEOF());
+  // But the body is not yet fully sent ("hello!" is not yet sent).
+  EXPECT_FALSE(stream->stream()->body_sent());
 
   data->ForceNextRead();   // Read in WINDOW_UPDATE frame.
   rv = callback.WaitForResult();
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index 432e3e51..0512dce 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -208,6 +208,9 @@ class NET_EXPORT_PRIVATE SpdyStream
   void Close();
   bool cancelled() const { return cancelled_; }
   bool closed() const { return io_state_ == STATE_DONE; }
+  // TODO(satorux): This is only for testing. We should be able to remove
+  // this once crbug.com/113107 is addressed.
+  bool body_sent() const { return io_state_ > STATE_SEND_BODY_COMPLETE; }
 
   // Interface for Spdy[Http|WebSocket]Stream to use.