Refactoring: Create per-connection packet writers in QuicDispatcher.

net/quic/quic_http_stream_test.cc

Index: net/quic/quic_http_stream_test.cc
diff --git a/net/quic/quic_http_stream_test.cc b/net/quic/quic_http_stream_test.cc
index 5e46258f2cbce5d46a4b131e3722ffcf0fdd7451..91cad75ef970b1759c62d27e91ed52d66475b508 100644
--- a/net/quic/quic_http_stream_test.cc
+++ b/net/quic/quic_http_stream_test.cc
@@ -58,12 +58,12 @@ class TestQuicConnection : public QuicConnection {
                      QuicConnectionId connection_id,
                      IPEndPoint address,
                      QuicConnectionHelper* helper,
-                     QuicPacketWriter* writer)
+                     const QuicConnection::PacketWriterFactory& writer_factory)
       : QuicConnection(connection_id,
                        address,
                        helper,
-                       writer,
-                       false  /* owns_writer */,
+                       writer_factory,
+                       true  /* owns_writer */,
                        false  /* is_server */,
                        versions) {
   }
@@ -102,6 +102,20 @@ class AutoClosingStream : public QuicHttpStream {
   }
 };
 
+class TestPacketWriterFactory : public QuicConnection::PacketWriterFactory {
+ public:
+  explicit TestPacketWriterFactory(DatagramClientSocket* socket)
+      : socket_(socket) {}
+  virtual ~TestPacketWriterFactory() {}
+
+  virtual QuicPacketWriter* Create(QuicConnection* connection) const OVERRIDE {
+    return new QuicDefaultPacketWriter(socket_);
+  }
+
+ private:
+  DatagramClientSocket* socket_;
+};
+
 }  // namespace
 
 class QuicHttpStreamPeer {
@@ -201,10 +215,10 @@ class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
     EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)).Times(AnyNumber());
     helper_.reset(new QuicConnectionHelper(runner_.get(), &clock_,
                                            &random_generator_));
-    writer_.reset(new QuicDefaultPacketWriter(socket));
+    TestPacketWriterFactory writer_factory(socket);

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

Note my use of a TestPacketWriterFactory local variable. This implies
QuicConnection can only use the PacketWriterFactory during the constructor call.
This API contract of the QuicConnection constructor should be documented.

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

The PacketWriterFactory is passed in as const& -- isn't that documentation
enough? It's theoretically possible to store a const& member or something
similar, but it's certainly against style. I think const& implicitly implies
that it should not be stored.

[wtc, 2014/08/14 20:05:00]

Hmm... you're right that is a very strong hint. I am worried that the
QuicConnection constructor may store that (const) reference in a data member and
use it later.
I searched for "reference" in the Style Guide. I found what supports your
statement:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

Under "Decision", it says:

 However, there are some instances where using const T* is preferable to const
T& for input parameters. For example:

    * You want to pass in a null pointer.
    * The function saves a pointer or reference to the input.

I think if the constructor may save a pointer or reference to the input
parameter, then we should signal that by using const T*.

     connection_ = new TestQuicConnection(SupportedVersions(GetParam()),
                                          connection_id_, peer_addr_,
-                                         helper_.get(), writer_.get());
+                                         helper_.get(), writer_factory);
     connection_->set_visitor(&visitor_);
     connection_->SetSendAlgorithm(send_algorithm_);
     connection_->SetReceiveAlgorithm(receive_algorithm_);
@@ -212,7 +226,7 @@ class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
     session_.reset(
         new QuicClientSession(connection_,
                               scoped_ptr<DatagramClientSocket>(socket),
-                              writer_.Pass(), NULL,
+                              NULL,
                               &crypto_client_stream_factory_,
                               make_scoped_ptr((QuicServerInfo*)NULL),
                               QuicServerId(kServerHostname, kServerPort,
@@ -298,7 +312,6 @@ class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
   scoped_ptr<QuicConnectionHelper> helper_;
   testing::StrictMock<MockConnectionVisitor> visitor_;
   scoped_ptr<QuicHttpStream> stream_;
-  scoped_ptr<QuicDefaultPacketWriter> writer_;
   scoped_ptr<QuicClientSession> session_;
   QuicCryptoClientConfig crypto_config_;
   TestCompletionCallback callback_;