Refactoring: Create per-connection packet writers in QuicDispatcher.

net/quic/quic_dispatcher.h

Index: net/quic/quic_dispatcher.h
diff --git a/net/quic/quic_dispatcher.h b/net/quic/quic_dispatcher.h
index 684461d9e1aa4f1df26c9c80231ff4f8d9aa4c73..c6f96d84adf8313c44e15102ffbecd479952d1be 100644
--- a/net/quic/quic_dispatcher.h
+++ b/net/quic/quic_dispatcher.h
@@ -22,7 +22,8 @@
 #include "net/quic/quic_server_session.h"
 #include "net/quic/quic_time_wait_list_manager.h"
 
-#if defined(COMPILER_GCC)
+#if defined(COMPILER_GCC) && !defined(QUIC_BLOCKED_WRITER_INTERFACE_HASH)
+#define QUIC_BLOCKED_WRITER_INTERFACE_HASH

[wtc, 2014/08/13 21:01:34]

IMPORTANT: this change is not ideal. We should try to find a better solution.

After looking into this, I suggest we move this hash<> template specialization
into the net/quic/quic_blocked_writer_interface.h header. This seems to
recommended by the comment at the top of base/containers/hash_tables.h:

// NOTE: It is an explicit non-goal of this class to provide a generic hash
// function for pointers.  If you want to hash a pointers to a particular class,
// please define the template specialization elsewhere (for example, in its
// header file) and keep it specific to just pointers to that class.  This is
// because identity hashes are not desirable for all types that might show up
// in containers as pointers.

[dmz, 2014/08/14 17:22:42]

Done.

 namespace BASE_HASH_NAMESPACE {
 template <>
 struct hash<net::QuicBlockedWriterInterface*> {
@@ -57,15 +58,40 @@ class QuicDispatcher : public QuicBlockedWriterInterface,
                        public QuicServerSessionVisitor,
                        public ProcessPacketInterface {
  public:
+  // Creates per-connection packet writers out of the QuicDispatcher's shared
+  // QuicPacketWriter. The per-connection writers' IsWriteBlocked() state must
+  // always be the same as the shared writer's IsWriteBlocked(), or else the
+  // QuicDispatcher::OnCanWrite logic will not work. (This will hopefully be
+  // cleaned up for bug 16950226.)
+  class PacketWriterFactory {
+   public:
+    virtual ~PacketWriterFactory() {}
+
+    virtual QuicPacketWriter* Create(QuicServerPacketWriter* writer,

[wtc, 2014/08/13 21:01:34]

IMPORTANT: please confirm that it is correct for these two Create() methods to
take a QuicServerPacketWriter* input and returns a QuicPacketWriter*. The
original code from the internal CL takes a QuicPacketWriter*.

[dmz, 2014/08/14 17:22:42]

Yes, that's right.

+                                     QuicConnection* connection) = 0;
+  };
+
+  // Creates ordinary QuicPerConnectionPacketWriter instances.
+  class DefaultPacketWriterFactory : public PacketWriterFactory {
+   public:
+    virtual ~DefaultPacketWriterFactory() {}
+
+    virtual QuicPacketWriter* Create(
+        QuicServerPacketWriter* writer,
+        QuicConnection* connection) OVERRIDE;
+  };
+
   // Ideally we'd have a linked_hash_set: the  boolean is unused.
   typedef linked_hash_map<QuicBlockedWriterInterface*, bool> WriteBlockedList;
 
-  // Due to the way delete_sessions_closure_ is registered, the Dispatcher
-  // must live until epoll_server Shutdown. |supported_versions| specifies the
-  // list of supported QUIC versions.
+  // Due to the way delete_sessions_closure_ is registered, the Dispatcher must
+  // live until epoll_server Shutdown. |supported_versions| specifies the list
+  // of supported QUIC versions. Takes ownership of |packet_writer_factory|,
+  // which is used to create per-connection writers.
   QuicDispatcher(const QuicConfig& config,
                  const QuicCryptoServerConfig& crypto_config,
                  const QuicVersionVector& supported_versions,
+                 PacketWriterFactory* packet_writer_factory,
                  QuicConnectionHelperInterface* helper);
 
   virtual ~QuicDispatcher();
@@ -113,8 +139,7 @@ class QuicDispatcher : public QuicBlockedWriterInterface,
   virtual QuicConnection* CreateQuicConnection(
       QuicConnectionId connection_id,
       const IPEndPoint& server_address,
-      const IPEndPoint& client_address,
-      QuicPerConnectionPacketWriter* writer);
+      const IPEndPoint& client_address);
 
   // Called by |framer_visitor_| when the public header has been parsed.
   virtual bool OnUnauthenticatedPublicHeader(
@@ -157,10 +182,29 @@ class QuicDispatcher : public QuicBlockedWriterInterface,
 
   QuicServerPacketWriter* writer() { return writer_.get(); }
 
+  const QuicConnection::PacketWriterFactory& connection_writer_factory() {
+    return connection_writer_factory_;
+  }
+
  private:
   class QuicFramerVisitor;
   friend class net::test::QuicDispatcherPeer;
 
+  // An adapter that creates packet writers using the dispatcher's
+  // PacketWriterFactory and shared writer. Essentially, it just curries the
+  // writer argument away from QuicDispatcher::PacketWriterFactory.
+  class PacketWriterFactoryAdapter :
+    public QuicConnection::PacketWriterFactory {
+   public:
+    PacketWriterFactoryAdapter(QuicDispatcher* dispatcher);
+    virtual ~PacketWriterFactoryAdapter ();
+
+    virtual QuicPacketWriter* Create(QuicConnection* connection) const OVERRIDE;
+
+   private:
+    QuicDispatcher* dispatcher_;
+  };
+
   // Called by |framer_visitor_| when the private header has been parsed
   // of a data packet that is destined for the time wait manager.
   void OnUnauthenticatedHeader(const QuicPacketHeader& header);
@@ -195,6 +239,12 @@ class QuicDispatcher : public QuicBlockedWriterInterface,
   // The writer to write to the socket with.
   scoped_ptr<QuicServerPacketWriter> writer_;
 
+  // Used to create per-connection packet writers, not |writer_| itself.
+  scoped_ptr<PacketWriterFactory> packet_writer_factory_;
+
+  // Passed in to QuicConnection for it to create the per-connection writers
+  PacketWriterFactoryAdapter connection_writer_factory_;
+
   // This vector contains QUIC versions which we currently support.
   // This should be ordered such that the highest supported version is the first
   // element, with subsequent elements in descending order (versions can be