Refactoring: Create per-connection packet writers in QuicDispatcher.

net/quic/test_tools/quic_test_utils.cc

Index: net/quic/test_tools/quic_test_utils.cc
diff --git a/net/quic/test_tools/quic_test_utils.cc b/net/quic/test_tools/quic_test_utils.cc
index c781b23c5e939b50c0f824a1d07a9b83edb9c208..df2e00725c9384eb3ebe96eb00a29982f2a4d6ec 100644
--- a/net/quic/test_tools/quic_test_utils.cc
+++ b/net/quic/test_tools/quic_test_utils.cc
@@ -4,6 +4,7 @@
 
 #include "net/quic/test_tools/quic_test_utils.h"
 
+#include "base/memory/singleton.h"
 #include "base/sha1.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -221,12 +222,34 @@ void MockHelper::AdvanceTime(QuicTime::Delta delta) {
   clock_.AdvanceTime(delta);
 }
 
+namespace {
+class NiceMockPacketWriterFactory
+    : public QuicConnection::PacketWriterFactory {
+ public:
+  virtual ~NiceMockPacketWriterFactory() {}
+
+  static NiceMockPacketWriterFactory* GetInstance() {
+    return Singleton<NiceMockPacketWriterFactory>::get();
+  }
+
+  virtual QuicPacketWriter* Create(
+      QuicConnection* /*connection*/) const override {
+    return new testing::NiceMock<MockPacketWriter>();
+  }
+
+ private:
+  friend struct DefaultSingletonTraits<NiceMockPacketWriterFactory>;
+  NiceMockPacketWriterFactory() {}
+  DISALLOW_COPY_AND_ASSIGN(NiceMockPacketWriterFactory);
+};
+}  // namespace
+
 MockConnection::MockConnection(bool is_server)
     : QuicConnection(kTestConnectionId,
                      IPEndPoint(TestPeerIPAddress(), kTestPort),
                      new testing::NiceMock<MockHelper>(),
-                     new testing::NiceMock<MockPacketWriter>(),
-                     true  /* owns_writer */,
+                     *NiceMockPacketWriterFactory::GetInstance(),

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

IMPORTANT: if we can rely on the API contract that QuicConnection will only use
the PacketWriterFactory during the constructor call, we can even pass a
temporary NiceMockPacketWriterFactory variable here. NiceMockPacketWriterFactory
doesn't have any data members, so it doesn't seem necessary to use it as a
singleton.

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

Good point, that would be simpler. I'll put this in the internal CL as well.

+                     /* owns_writer= */ true,
                      is_server, QuicSupportedVersions()),
       helper_(helper()) {
 }
@@ -235,8 +258,8 @@ MockConnection::MockConnection(IPEndPoint address,
                                bool is_server)
     : QuicConnection(kTestConnectionId, address,
                      new testing::NiceMock<MockHelper>(),
-                     new testing::NiceMock<MockPacketWriter>(),
-                     true  /* owns_writer */,
+                     *NiceMockPacketWriterFactory::GetInstance(),
+                     /* owns_writer= */ true,
                      is_server, QuicSupportedVersions()),
       helper_(helper()) {
 }
@@ -246,8 +269,8 @@ MockConnection::MockConnection(QuicConnectionId connection_id,
     : QuicConnection(connection_id,
                      IPEndPoint(TestPeerIPAddress(), kTestPort),
                      new testing::NiceMock<MockHelper>(),
-                     new testing::NiceMock<MockPacketWriter>(),
-                     true  /* owns_writer */,
+                     *NiceMockPacketWriterFactory::GetInstance(),
+                     /* owns_writer= */ true,
                      is_server, QuicSupportedVersions()),
       helper_(helper()) {
 }
@@ -257,8 +280,8 @@ MockConnection::MockConnection(bool is_server,
     : QuicConnection(kTestConnectionId,
                      IPEndPoint(TestPeerIPAddress(), kTestPort),
                      new testing::NiceMock<MockHelper>(),
-                     new testing::NiceMock<MockPacketWriter>(),
-                     true  /* owns_writer */,
+                     *NiceMockPacketWriterFactory::GetInstance(),
+                     /* owns_writer= */ true,
                      is_server, supported_versions),
       helper_(helper()) {
 }
@@ -606,5 +629,53 @@ QuicVersionVector SupportedVersions(QuicVersion version) {
   return versions;
 }
 
+TestWriterFactory::TestWriterFactory() : current_writer_(NULL) {}
+TestWriterFactory::~TestWriterFactory() {}
+
+QuicPacketWriter* TestWriterFactory::Create(QuicServerPacketWriter* writer,
+                                            QuicConnection* connection) {
+  return new PerConnectionPacketWriter(this, writer, connection);
+}
+
+void TestWriterFactory::OnPacketSent(WriteResult result) {
+  if (current_writer_ != NULL) {
+    current_writer_->connection()->OnPacketSent(result);
+    current_writer_ = NULL;
+  }
+}
+
+void TestWriterFactory::Unregister(PerConnectionPacketWriter* writer) {
+  if (current_writer_ == writer) {
+    current_writer_ = NULL;
+  }
+}
+
+TestWriterFactory::PerConnectionPacketWriter::PerConnectionPacketWriter(
+    TestWriterFactory* factory,
+    QuicServerPacketWriter* writer,
+    QuicConnection* connection)
+    : QuicPerConnectionPacketWriter(writer, connection),
+      factory_(factory) {
+}
+
+TestWriterFactory::PerConnectionPacketWriter::~PerConnectionPacketWriter() {
+  factory_->Unregister(this);
+}
+
+WriteResult TestWriterFactory::PerConnectionPacketWriter::WritePacket(
+    const char* buffer,
+    size_t buf_len,
+    const IPAddressNumber& self_address,
+    const IPEndPoint& peer_address) {
+  // A DCHECK(factory_current_writer_ == NULL) would be wrong here -- this class
+  // may be used in a setting where connection()->OnPacketSent() is called in a
+  // different way, so TestWriterFactory::OnPacketSent might never be called.
+  factory_->current_writer_ = this;
+  return QuicPerConnectionPacketWriter::WritePacket(buffer,
+                                                    buf_len,
+                                                    self_address,
+                                                    peer_address);
+}
+
 }  // namespace test
 }  // namespace net