SPDY: WriteHeaders should invoke OnDataSent callback

Introduce a completion waiting list, and check which kind of frame is sent.
OnDataSent() is invoked iff the frame wis data frame.

BUG=42320
TEST=net_unittests


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

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

14 files changed, 77 insertions, 30 deletions

diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 3c40a1a..e65dc45 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -411,6 +411,11 @@ int SpdyHttpStream::OnResponseReceived(const SpdyHeaderBlock& response,
   return status;
 }
 
+void SpdyHttpStream::OnHeadersSent() {
+  // For HTTP streams, no HEADERS frame is sent from the client.
+  NOTREACHED();
+}
+
 int SpdyHttpStream::OnDataReceived(const char* data, int length) {
   // SpdyStream won't call us with data if the header block didn't contain a
   // valid set of headers.  So we don't expect to not have headers received
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index a0fedaf..21e124f 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -81,6 +81,7 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate,
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
+  virtual void OnHeadersSent() OVERRIDE;
   virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc
index 32e1400..0d62ce7 100644
--- a/net/spdy/spdy_proxy_client_socket.cc
+++ b/net/spdy/spdy_proxy_client_socket.cc
@@ -512,6 +512,11 @@ int SpdyProxyClientSocket::OnResponseReceived(
   return OK;
 }
 
+void SpdyProxyClientSocket::OnHeadersSent() {
+  // Proxy client sockets don't send any HEADERS frame.
+  NOTREACHED();
+}
+
 // Called when data is received.
 int SpdyProxyClientSocket::OnDataReceived(const char* data, int length) {
   if (length > 0) {
diff --git a/net/spdy/spdy_proxy_client_socket.h b/net/spdy/spdy_proxy_client_socket.h
index 0951f2a..a5da281 100644
--- a/net/spdy/spdy_proxy_client_socket.h
+++ b/net/spdy/spdy_proxy_client_socket.h
@@ -97,6 +97,7 @@ class NET_EXPORT_PRIVATE SpdyProxyClientSocket : public ProxyClientSocket,
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
+  virtual void OnHeadersSent() OVERRIDE;
   virtual int OnDataReceived(const char* data, int length) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc
index ecddf9b..15d78e0 100644
--- a/net/spdy/spdy_session_spdy2_unittest.cc
+++ b/net/spdy/spdy_session_spdy2_unittest.cc
@@ -43,6 +43,7 @@ class ClosingDelegate : public SpdyStream::Delegate {
                                  int status) OVERRIDE {
     return OK;
   }
+  virtual void OnHeadersSent() OVERRIDE {}
   virtual int OnDataReceived(const char* data, int length) OVERRIDE {
     return OK;
   }
@@ -74,30 +75,31 @@ class TestSpdyStreamDelegate : public net::SpdyStream::Delegate {
       : callback_(callback) {}
   virtual ~TestSpdyStreamDelegate() {}
 
-  virtual bool OnSendHeadersComplete(int status) { return true; }
+  virtual bool OnSendHeadersComplete(int status) OVERRIDE { return true; }
 
-  virtual int OnSendBody() {
+  virtual int OnSendBody() OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
-  virtual int OnSendBodyComplete(int /*status*/, bool* /*eof*/) {
+  virtual int OnSendBodyComplete(int /*status*/, bool* /*eof*/) OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
-                                 int status) {
+                                 int status) OVERRIDE {
     return status;
   }
 
-  virtual int OnDataReceived(const char* buffer, int bytes) {
+  virtual void OnHeadersSent() OVERRIDE {}
+
+  virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE {
     return OK;
   }
 
-  virtual void OnDataSent(int length) {
-  }
+  virtual void OnDataSent(int length) OVERRIDE {}
 
-  virtual void OnClose(int status) {
+  virtual void OnClose(int status) OVERRIDE {
     CompletionCallback callback = callback_;
     callback_.Reset();
     callback.Run(OK);
diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc
index 54c0398..b60c172 100644
--- a/net/spdy/spdy_session_spdy3_unittest.cc
+++ b/net/spdy/spdy_session_spdy3_unittest.cc
@@ -43,6 +43,7 @@ class ClosingDelegate : public SpdyStream::Delegate {
                                  int status) OVERRIDE {
     return OK;
   }
+  virtual void OnHeadersSent() OVERRIDE {}
   virtual int OnDataReceived(const char* data, int length) OVERRIDE {
     return OK;
   }
@@ -55,36 +56,36 @@ class ClosingDelegate : public SpdyStream::Delegate {
 };
 
 
-class TestSpdyStreamDelegate : public net::SpdyStream::Delegate {
+class TestSpdyStreamDelegate : public SpdyStream::Delegate {
  public:
   explicit TestSpdyStreamDelegate(const CompletionCallback& callback)
       : callback_(callback) {}
   virtual ~TestSpdyStreamDelegate() {}
 
-  virtual bool OnSendHeadersComplete(int status) { return true; }
+  virtual bool OnSendHeadersComplete(int status) OVERRIDE { return true; }
 
-  virtual int OnSendBody() {
+  virtual int OnSendBody() OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
-  virtual int OnSendBodyComplete(int /*status*/, bool* /*eof*/) {
+  virtual int OnSendBodyComplete(int /*status*/, bool* /*eof*/) OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
-                                 int status) {
+                                 int status) OVERRIDE {
     return status;
   }
 
-  virtual int OnDataReceived(const char* buffer, int bytes) {
+  virtual void OnHeadersSent() OVERRIDE {}
+  virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE {
     return OK;
   }
 
-  virtual void OnDataSent(int length) {
-  }
+  virtual void OnDataSent(int length) OVERRIDE {}
 
-  virtual void OnClose(int status) {
+  virtual void OnClose(int status) OVERRIDE {
     CompletionCallback callback = callback_;
     callback_.Reset();
     callback.Run(OK);
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 1e2db2c..aeb370e 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -142,11 +142,13 @@ SpdyFrame* SpdyStream::ProduceNextFrame() {
     PendingFrame frame = pending_frames_.front();
     pending_frames_.pop_front();
 
+    waiting_completions_.push_back(frame.type);
+
     if (frame.type == TYPE_DATA) {
       // Send queued data frame.
       return frame.data_frame;
     } else {
-      DCHECK(frame.type == TYPE_HEADER);
+      DCHECK(frame.type == TYPE_HEADERS);
       // Create actual HEADERS frame just in time because it depends on
       // compression context and should not be reordered after the creation.
       SpdyFrame* header_frame = session_->CreateHeadersFrame(
@@ -589,7 +591,7 @@ int SpdyStream::WriteHeaders(SpdyHeaderBlock* headers) {
   CHECK_GT(stream_id_, 0u);
 
   PendingFrame frame;
-  frame.type = TYPE_HEADER;
+  frame.type = TYPE_HEADERS;
   frame.header_block = headers;
   pending_frames_.push_back(frame);
 
@@ -831,8 +833,16 @@ int SpdyStream::DoSendBodyComplete(int result) {
 }
 
 int SpdyStream::DoOpen(int result) {
-  if (delegate_)
-    delegate_->OnDataSent(result);
+  if (delegate_) {
+    FrameType type = waiting_completions_.front();
+    waiting_completions_.pop_front();
+    if (type == TYPE_DATA) {
+      delegate_->OnDataSent(result);
+    } else {
+      DCHECK(type == TYPE_HEADERS);
+      delegate_->OnHeadersSent();
+    }
+  }
   io_state_ = STATE_OPEN;
   return result;
 }
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index f72f147..e368bab 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -71,6 +71,9 @@ class NET_EXPORT_PRIVATE SpdyStream
                                    base::Time response_time,
                                    int status) = 0;
 
+    // Called when a HEADERS frame is sent.
+    virtual void OnHeadersSent() = 0;
+
     // Called when data is received.
     // Returns network error code. OK when it successfully receives data.
     virtual int OnDataReceived(const char* data, int length) = 0;
@@ -90,14 +93,14 @@ class NET_EXPORT_PRIVATE SpdyStream
   };
 
   // Indicates pending frame type.
-  enum PendingFrameType {
-    TYPE_HEADER,
+  enum FrameType {
+    TYPE_HEADERS,
     TYPE_DATA
   };
 
   // Structure to contains pending frame information.
   typedef struct {
-    PendingFrameType type;
+    FrameType type;
     union {
       SpdyHeaderBlock* header_block;
       SpdyDataFrame* data_frame;
@@ -365,8 +368,15 @@ class NET_EXPORT_PRIVATE SpdyStream
   scoped_ptr<SpdyHeaderBlock> response_;
   base::Time response_time_;
 
+  // An in order list of pending frame data that are going to be sent. HEADERS
+  // frames are queued as SpdyHeaderBlock structures because these must be
+  // compressed just before sending. Data frames are queued as SpdyDataFrame.
   std::list<PendingFrame> pending_frames_;
 
+  // An in order list of sending frame types. It will be used to know which type
+  // of frame is sent and which callback should be invoked in OnOpen().
+  std::list<FrameType> waiting_completions_;
+
   State io_state_;
 
   // Since we buffer the response, we also buffer the response status.
diff --git a/net/spdy/spdy_stream_spdy2_unittest.cc b/net/spdy/spdy_stream_spdy2_unittest.cc
index 8055fd8..37bc392 100644
--- a/net/spdy/spdy_stream_spdy2_unittest.cc
+++ b/net/spdy/spdy_stream_spdy2_unittest.cc
@@ -266,10 +266,9 @@ TEST_F(SpdyStreamSpdy2Test, SendHeaderAndDataAfterOpen) {
 
   EXPECT_TRUE(delegate->send_headers_completed());
   EXPECT_EQ("101", (*delegate->response())["status"]);
+  EXPECT_EQ(1, delegate->headers_sent());
   EXPECT_EQ(std::string(), delegate->received_data());
-  // TODO(toyoshim): OnDataSent should be invoked when each data frame is sent.
-  // But current implementation invokes also when each HEADERS frame is sent.
-  //EXPECT_EQ(6, delegate->data_sent());
+  EXPECT_EQ(6, delegate->data_sent());
 }
 
 TEST_F(SpdyStreamSpdy2Test, PushedStream) {
diff --git a/net/spdy/spdy_stream_spdy3_unittest.cc b/net/spdy/spdy_stream_spdy3_unittest.cc
index 3a3ad66..313756a 100644
--- a/net/spdy/spdy_stream_spdy3_unittest.cc
+++ b/net/spdy/spdy_stream_spdy3_unittest.cc
@@ -267,10 +267,9 @@ TEST_F(SpdyStreamSpdy3Test, SendHeaderAndDataAfterOpen) {
 
   EXPECT_TRUE(delegate->send_headers_completed());
   EXPECT_EQ("101", (*delegate->response())[":status"]);
+  EXPECT_EQ(1, delegate->headers_sent());
   EXPECT_EQ(std::string(), delegate->received_data());
-  // TODO(toyoshim): OnDataSent should be invoked when each data frame is sent.
-  // But current implementation invokes also when each HEADERS frame is sent.
-  //EXPECT_EQ(6, delegate->data_sent());
+  EXPECT_EQ(6, delegate->data_sent());
 }
 
 TEST_F(SpdyStreamSpdy3Test, PushedStream) {
diff --git a/net/spdy/spdy_stream_test_util.cc b/net/spdy/spdy_stream_test_util.cc
index c39c46c..e2144ae 100644
--- a/net/spdy/spdy_stream_test_util.cc
+++ b/net/spdy/spdy_stream_test_util.cc
@@ -23,6 +23,7 @@ TestSpdyStreamDelegate::TestSpdyStreamDelegate(
       callback_(callback),
       send_headers_completed_(false),
       response_(new SpdyHeaderBlock),
+      headers_sent_(0),
       data_sent_(0),
       closed_(false) {
 }
@@ -61,6 +62,10 @@ int TestSpdyStreamDelegate::OnResponseReceived(const SpdyHeaderBlock& response,
   return status;
 }
 
+void TestSpdyStreamDelegate::OnHeadersSent() {
+  headers_sent_++;
+}
+
 int TestSpdyStreamDelegate::OnDataReceived(const char* buffer, int bytes) {
   received_data_ += std::string(buffer, bytes);
   return OK;
diff --git a/net/spdy/spdy_stream_test_util.h b/net/spdy/spdy_stream_test_util.h
index 2bf2936..6059014 100644
--- a/net/spdy/spdy_stream_test_util.h
+++ b/net/spdy/spdy_stream_test_util.h
@@ -28,6 +28,7 @@ class TestSpdyStreamDelegate : public SpdyStream::Delegate {
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
+  virtual void OnHeadersSent() OVERRIDE;
   virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
@@ -37,6 +38,7 @@ class TestSpdyStreamDelegate : public SpdyStream::Delegate {
     return response_;
   }
   const std::string& received_data() const { return received_data_; }
+  int headers_sent() const { return headers_sent_; }
   int data_sent() const { return data_sent_; }
   bool closed() const {  return closed_; }
 
@@ -48,6 +50,7 @@ class TestSpdyStreamDelegate : public SpdyStream::Delegate {
   bool send_headers_completed_;
   linked_ptr<SpdyHeaderBlock> response_;
   std::string received_data_;
+  int headers_sent_;
   int data_sent_;
   bool closed_;
 
diff --git a/net/spdy/spdy_websocket_stream.cc b/net/spdy/spdy_websocket_stream.cc
index 32248ad..6323b71 100644
--- a/net/spdy/spdy_websocket_stream.cc
+++ b/net/spdy/spdy_websocket_stream.cc
@@ -107,6 +107,11 @@ int SpdyWebSocketStream::OnResponseReceived(
   return delegate_->OnReceivedSpdyResponseHeader(response, status);
 }
 
+void SpdyWebSocketStream::OnHeadersSent() {
+  // This will be called when WebSocket over SPDY supports new framing.
+  NOTREACHED();
+}
+
 int SpdyWebSocketStream::OnDataReceived(const char* data, int length) {
   DCHECK(delegate_);
   delegate_->OnReceivedSpdyData(data, length);
diff --git a/net/spdy/spdy_websocket_stream.h b/net/spdy/spdy_websocket_stream.h
index 58c62b02..a3a2314 100644
--- a/net/spdy/spdy_websocket_stream.h
+++ b/net/spdy/spdy_websocket_stream.h
@@ -77,6 +77,7 @@ class NET_EXPORT_PRIVATE SpdyWebSocketStream
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
+  virtual void OnHeadersSent() OVERRIDE;
   virtual int OnDataReceived(const char* data, int length) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;