[SPDY] Refactor SpdyStream's handling of response headers

net/spdy/spdy_stream.h

Index: net/spdy/spdy_stream.h
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index d2f3de72f1179e0c3df7506de45b41c02fb7cf63..2db456aae6330426365f8f896d67fbdb18485cef 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -73,19 +73,46 @@ class NET_EXPORT_PRIVATE SpdyStream {
     // for push streams.
     virtual void OnRequestHeadersSent() = 0;
 
-    // Called when the SYN_STREAM, SYN_REPLY, or HEADERS frames are received.
-    // Normal streams will receive a SYN_REPLY and optional HEADERS frames.
-    // Pushed streams will receive a SYN_STREAM and optional HEADERS frames.
-    // Because a stream may have a SYN_* frame and multiple HEADERS frames,
-    // this callback may be called multiple times.
-    // |status| indicates network error. Returns network error code.
-    virtual int OnResponseHeadersReceived(const SpdyHeaderBlock& response,
-                                          base::Time response_time,
-                                          int status) = 0;
+    // Called when the response headers are updated from the
+    // server. |headers| contains the set of all headers received up
+    // to this point; delegates can assume that any headers previously
+    // received remain unchanged.
+    //
+    // This is called at least once before any data is
+    // received. After that:
+    //
+    // - for bidirectional streams, this can be called again any
+    //   number of times interleaved with received data;
+    // - for request/response streams, this will never be called
+    //   again;
+    // - and for pushed streams, this can be called again, but only
+    //   until data is received.
+    //
+    // The semantics of the return value are a bit confusing:
+    //
+    // - This function must return either OK or
+    //   ERR_INCOMPLETE_SPDY_HEADERS.

[Ryan Hamilton, 2013/06/19 18:58:36]

nit: if there are only 2 return values, should it return an enum instead?

[akalin, 2013/06/21 21:13:25]

Done! Also use the enum in various other places.

+    //
+    // - If OK is returned, the delegate may still have closed the
+    //   stream; OK means that the headers were processed
+    //   successfully, but the headers may indicate an error condition
+    //   that results in the closing of the stream.

[Ryan Hamilton, 2013/06/19 18:58:36]

nit: can you rephrase this to something like:
- If OK is returned, the delegate has processed the headers successfully. 
However, ...

[akalin, 2013/06/21 21:13:25]

Done.

+    //
+    // - If ERR_INCOMPLETE_SPDY_HEADERS is returned, the delegate must
+    //   not have closed the stream. For non-push streams, this is
+    //   interpreted as an error condition and the stream will be
+    //   closed. However, for push streams, this return value is not
+    //   interpreted as an error condition, since additional headers
+    //   may still be received.
+    virtual int OnResponseHeadersUpdated(
+        const SpdyHeaderBlock& response_headers) = 0;
 
     // Called when data is received. |buffer| may be NULL, which
     // signals EOF.  Must return OK if the data was received
     // successfully, or a network error code otherwise.
+    //
+    // May cause the stream to be closed regardless of the return
+    // value.
     virtual int OnDataReceived(scoped_ptr<SpdyBuffer> buffer) = 0;
 
     // Called when data is sent.
@@ -115,23 +142,29 @@ class NET_EXPORT_PRIVATE SpdyStream {
   ~SpdyStream();
 
   // Set new |delegate|. |delegate| must not be NULL.  If it already
-  // received SYN_REPLY or data, OnResponseHeadersReceived() or
+  // received SYN_REPLY or data, OnResponseHeadersUpdated() or
   // OnDataReceived() will be called.
   void SetDelegate(Delegate* delegate);
-  Delegate* GetDelegate() { return delegate_; }
+  Delegate* GetDelegate();
 
   // Detach the delegate from the stream, which must not yet be
   // closed, and cancel it.
   void DetachDelegate();
 
+  // Whether or not the initial response headers have been received
+  // from the server.
+  bool ReceivedInitialResponseHeaders() const;

[Ryan Hamilton, 2013/06/19 18:58:36]

nit: HasReceivedInitialResponseHeaders

[akalin, 2013/06/21 21:13:25]

Done.

+
+  // The time at which the first bytes of the response were received
+  // from the server. Non-null only when ReceivedResponseHeaders()
+  // returns true.
+  base::Time response_time() const { return response_time_; }
+
   SpdyStreamType type() const { return type_; }
 
   SpdyStreamId stream_id() const { return stream_id_; }
   void set_stream_id(SpdyStreamId stream_id) { stream_id_ = stream_id; }
 
-  bool response_received() const { return response_received_; }
-  void set_response_received() { response_received_ = true; }
-
   const std::string& path() const { return path_; }
 
   RequestPriority priority() const { return priority_; }
@@ -228,14 +261,27 @@ class NET_EXPORT_PRIVATE SpdyStream {
   base::Time GetRequestTime() const;
   void SetRequestTime(base::Time t);
 
-  // Called by the SpdySession when a response (e.g. a SYN_STREAM or
-  // SYN_REPLY) has been received for this stream. This is the entry
-  // point for a push stream. Returns a status code.
-  int OnResponseHeadersReceived(const SpdyHeaderBlock& response);
-
-  // Called by the SpdySession when late-bound headers are received for a
-  // stream. Returns a status code.
-  int OnHeaders(const SpdyHeaderBlock& headers);
+  // Called at most once by the SpdySession when the initial response
+  // headers have been received for this stream, i.e., a SYN_REPLY (or
+  // SYN_STREAM for push streams) frame has been received. This is the
+  // entry point for a push stream. Returns a status code; if it is an
+  // error, the stream may have already been closed.
+  //
+  // TODO(akalin): Guarantee that the stream is already closed if an
+  // error is returned.
+  int OnInitialResponseHeadersReceived(const SpdyHeaderBlock& response_headers,
+                                       base::Time response_time,
+                                       base::TimeTicks recv_first_byte_time);
+
+  // Called by the SpdySession (only after
+  // OnInitialResponseHeadersReceived() has been called) when
+  // late-bound headers are received for a stream. Returns a status
+  // code; if it is an error, ths stream may have already been closed.
+  //
+  // TODO(akalin): Guarantee that the stream is already closed if an
+  // error is returned.
+  int OnAdditionalResponseHeadersReceived(
+      const SpdyHeaderBlock& additional_response_headers);
 
   // Called by the SpdySession when response data has been received
   // for this stream.  This callback may be called multiple times as
@@ -289,7 +335,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // bidirectional streams; for request/response streams, it must be
   // MORE_DATA_TO_SEND if the request has data to upload, or
   // NO_MORE_DATA_TO_SEND if not.
-  int SendRequestHeaders(scoped_ptr<SpdyHeaderBlock> headers,
+  int SendRequestHeaders(scoped_ptr<SpdyHeaderBlock> request_headers,
                          SpdySendStatus send_status);
 
   // Sends a DATA frame. The delegate will be notified via
@@ -369,8 +415,9 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // be called after the stream has completed.
   void UpdateHistograms();
 
-  // When a server pushed stream is first created, this function is posted on
-  // the MessageLoop to replay all the data that the server has already sent.
+  // When a server-pushed stream is first created, this function is
+  // posted on the current MessageLoop to replay the data that the
+  // server has already sent.
   void PushedStreamReplayData();
 
   // Produces the SYN_STREAM frame for the stream. The stream must
@@ -387,6 +434,15 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // |pending_send_data_| is set.
   void QueueNextDataFrame();
 
+  // Merge the given headers into |response_headers_| and calls
+  // OnResponseHeadersUpdated() on the delegate (if attached).
+  // Returns a status code; if it is an error, the stream may have
+  // already been closed.
+  //
+  // TODO(akalin): Guarantee that the stream is already closed if an
+  // error is returned.
+  int MergeWithResponseHeaders(const SpdyHeaderBlock& new_response_headers);
+
   const SpdyStreamType type_;
 
   base::WeakPtrFactory<SpdyStream> weak_ptr_factory_;
@@ -412,7 +468,6 @@ class NET_EXPORT_PRIVATE SpdyStream {
   int32 unacked_recv_window_bytes_;
 
   ScopedBandwidthMetrics metrics_;
-  bool response_received_;
 
   scoped_refptr<SpdySession> session_;
 
@@ -426,7 +481,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   //
   // TODO(akalin): Hang onto this only until we send it. This
   // necessitates stashing the URL separately.
-  scoped_ptr<SpdyHeaderBlock> request_;
+  scoped_ptr<SpdyHeaderBlock> request_headers_;
 
   // The data waiting to be sent.
   scoped_refptr<DrainableIOBuffer> pending_send_data_;
@@ -435,7 +490,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // For cached responses, this time could be "far" in the past.
   base::Time request_time_;
 
-  scoped_ptr<SpdyHeaderBlock> response_;
+  scoped_ptr<SpdyHeaderBlock> response_headers_;
   base::Time response_time_;
 
   State io_state_;