[SPDY] Refactor SpdySession's write queue

net/spdy/spdy_session.h

Index: net/spdy/spdy_session.h
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 0d52b905bbac1406a05e0f660f82576170cb8cdc..4757bf991b5dba4fc79220972716c46b5170a736 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -6,20 +6,20 @@
 #define NET_SPDY_SPDY_SESSION_H_
 
 #include <deque>
-#include <list>
 #include <map>
-#include <queue>
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_states.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_export.h"
 #include "net/base/request_priority.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/ssl_client_socket.h"
@@ -164,6 +164,19 @@ class NET_EXPORT_PRIVATE SpdyStreamRequest {
   DISALLOW_COPY_AND_ASSIGN(SpdyStreamRequest);
 };
 
+// The interface for something which provides a SpdyFrame for writing.
+class NET_EXPORT_PRIVATE SpdyFrameProducer {
+public:
+  // Returns the SpdyStream associated with the frame to be produced,
+  // or NULL if the frame is associated with the session itself.
+  virtual SpdyStream* GetStream() = 0;

[Ryan Hamilton, 2013/03/24 15:22:54]

See this comment on the CL which landed SpdyIOBufferProducer:

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...

I had such a method in my initial implementation, but willchan requested it be
removed.  Can you refactor accordingly?

[akalin, 2013/03/24 18:24:14]

Hmm I looked at the comment but willchan didn't state his reason.

I think it's simpler to have this method, as it avoids the need for the extra
map and lets us move the activate-if-necessary logic back to the SpdySession.

willchan, can you elaborate on why you don't like this method?

[willchan no longer on Chromium, 2013/03/24 18:58:22]

Layering. This is at the frame level, which is lower level than streams. It
feels like a minor layering violation (kind of like RTTI), especially since not
all frames have streams. A HasStream() or GetStreamId() wouldn be less of a
violation that exposing a raw SpdyStream*.

Now, layers are obviously arbitrary. Oftentimes tighter coupling and removal of
abstraction layers can simplify code. There are tradeoffs here. I generally err
on the side of maintaining abstraction boundaries throughout most of my code
unless it's obviously too cumbersome and would be much simpler to eliminate the
arbitrary mental layer.

I leave that up to you guys. 

+
+  // Produces the frame to be written. Will be called at most once.
+  virtual scoped_ptr<SpdyFrame> ProduceFrame() = 0;
+
+  virtual ~SpdyFrameProducer() {}
+};
+
 class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
                                public BufferedSpdyFramerVisitorInterface {
  public:
@@ -177,29 +190,6 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
     FLOW_CONTROL_STREAM_AND_SESSION
   };
 
-  // Defines an interface for producing SpdyIOBuffers.
-  class NET_EXPORT_PRIVATE SpdyIOBufferProducer {
-   public:
-    SpdyIOBufferProducer() {}
-
-    // Returns a newly created SpdyIOBuffer, owned by the caller, or NULL
-    // if not buffer is ready to be produced.
-    virtual SpdyIOBuffer* ProduceNextBuffer(SpdySession* session) = 0;
-
-    virtual RequestPriority GetPriority() const = 0;
-
-    virtual ~SpdyIOBufferProducer() {}
-
-   protected:
-    // Activates |spdy_stream| in |spdy_session|.
-    static void ActivateStream(SpdySession* spdy_session,
-                               SpdyStream* spdy_stream);
-
-    static SpdyIOBuffer* CreateIOBuffer(SpdyFrame* frame,
-                                        RequestPriority priority,
-                                        SpdyStream* spdy_stream);
-  };
-
   // Create a new SpdySession.
   // |host_port_proxy_pair| is the host/port that this session connects to, and
   // the proxy configuration settings that it's using.
@@ -263,9 +253,8 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
   bool VerifyDomainAuthentication(const std::string& domain);
 
   // Records that |stream| has a write available from |producer|.
-  // |producer| will be owned by this SpdySession.
   void SetStreamHasWriteAvailable(SpdyStream* stream,
-                                  SpdyIOBufferProducer* producer);
+                                  scoped_ptr<SpdyFrameProducer> producer);
 
   // Send the SYN frame for |stream_id|. This also sends PING message to check
   // the status of the connection.
@@ -475,19 +464,6 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
       std::pair<scoped_refptr<SpdyStream>, base::TimeTicks> > PushedStreamMap;
 
   typedef std::set<scoped_refptr<SpdyStream> > CreatedStreamSet;
-  typedef std::map<SpdyIOBufferProducer*, SpdyStream*> StreamProducerMap;
-
-  class SpdyIOBufferProducerCompare {
-   public:
-    bool operator() (const SpdyIOBufferProducer* lhs,
-                     const SpdyIOBufferProducer* rhs) const {
-      return lhs->GetPriority() < rhs->GetPriority();
-    }
-  };
-
-  typedef std::priority_queue<SpdyIOBufferProducer*,
-                              std::vector<SpdyIOBufferProducer*>,
-                              SpdyIOBufferProducerCompare> WriteQueue;
 
   enum State {
     STATE_IDLE,
@@ -582,10 +558,28 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
   // Get a new stream id.
   int GetNewStreamId();
 
-  // Queue a frame for sending.
+  // Delegates to QueueFrameProducerForWriting to put the given frame
+  // associated with the session on the write queue.
+  //
   // |frame| is the frame to send.
   // |priority| is the priority for insertion into the queue.
-  void QueueFrame(SpdyFrame* frame, RequestPriority priority);
+  void QueueSessionFrameForWriting(scoped_ptr<SpdyFrame> frame,
+                                   RequestPriority priority);
+
+  // Puts |producer| onto the write queue with the given priority.
+  void QueueFrameProducerForWriting(scoped_ptr<SpdyFrameProducer> producer,
+                                    RequestPriority priority);
+
+  // Pops the next frame producer to send from the write queue, or
+  // NULL if there isn't one.
+  scoped_ptr<SpdyFrameProducer> PopNextFrameProducerToWrite();
+
+  // Remove all frame producers associated with the given stream from
+  // the write queue.
+  void RemoveStreamFromWriteQueue(const scoped_refptr<SpdyStream>& stream);
+
+  // Remove everything from the write queue.
+  void ClearWriteQueue();
 
   // Track active streams in the active stream list.
   void ActivateStream(SpdyStream* stream);
@@ -768,13 +762,8 @@ class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
   // Set of all created streams but that have not yet sent any frames.
   CreatedStreamSet created_streams_;
 
-  // As streams have data to be sent, we put them into the write queue.
-  WriteQueue write_queue_;
-
-  // Mapping from SpdyIOBufferProducers to their corresponding SpdyStream
-  // so that when a stream is destroyed, we can remove the corresponding
-  // producer from |write_queue_|.
-  StreamProducerMap stream_producers_;
+  // A FIFO queue of data to be sent, binned by priority.
+  std::deque<SpdyFrameProducer*> write_queue_[NUM_PRIORITIES];
 
   // The packet we are currently sending.
   bool write_pending_;            // Will be true when a write is in progress.