Refactoring: Create per-connection packet writers in QuicDispatcher.

net/quic/quic_connection_test.cc

Index: net/quic/quic_connection_test.cc
diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc
index 7faafe7a844ba689c1f2664b62b5839494aaaaec..e22a9defbe5831e1109f496666130050132b57df 100644
--- a/net/quic/quic_connection_test.cc
+++ b/net/quic/quic_connection_test.cc
@@ -38,6 +38,7 @@ using testing::Contains;
 using testing::DoAll;
 using testing::InSequence;
 using testing::InvokeWithoutArgs;
+using testing::NiceMock;
 using testing::Ref;
 using testing::Return;
 using testing::SaveArg;
@@ -412,21 +413,20 @@ class TestConnection : public QuicConnection {
   TestConnection(QuicConnectionId connection_id,
                  IPEndPoint address,
                  TestConnectionHelper* helper,
-                 TestPacketWriter* writer,
+                 const PacketWriterFactory& factory,
                  bool is_server,
                  QuicVersion version)
       : QuicConnection(connection_id,
                        address,
                        helper,
-                       writer,
-                       false  /* owns_writer */,
+                       factory,
+                       /* owns_writer= */ false,
                        is_server,
-                       SupportedVersions(version)),
-        writer_(writer) {
+                       SupportedVersions(version)) {
     // Disable tail loss probes for most tests.
     QuicSentPacketManagerPeer::SetMaxTailLossProbes(
         QuicConnectionPeer::GetSentPacketManager(this), 0);
-    writer_->set_is_server(is_server);
+    writer()->set_is_server(is_server);

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

Nit: the change of |writer_| to |writer()| seems unnecessary -- we should be
able to just use |writer_|, right?

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

I don't think |writer_| exists anymore, right?

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

You are right. You deleted the |writer_| member from this class. The |writer_|
member of the base class QuicConnection is private, so it's not accessible.

   }
 
   void SendAck() {
@@ -538,11 +538,11 @@ class TestConnection : public QuicConnection {
 
   void SetSupportedVersions(const QuicVersionVector& versions) {
     QuicConnectionPeer::GetFramer(this)->SetSupportedVersions(versions);
-    writer_->SetSupportedVersions(versions);
+    writer()->SetSupportedVersions(versions);
   }
 
   void set_is_server(bool is_server) {
-    writer_->set_is_server(is_server);
+    writer()->set_is_server(is_server);
     QuicConnectionPeer::SetIsServer(this, is_server);
   }
 
@@ -579,7 +579,9 @@ class TestConnection : public QuicConnection {
   using QuicConnection::SelectMutualVersion;
 
  private:
-  TestPacketWriter* writer_;
+  TestPacketWriter* writer() {
+    return reinterpret_cast<TestPacketWriter*>(QuicConnection::writer());

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

IMPORTANT: please verify that it is fine to use reinterpret_cast here. The
original code from the internal CL uses down_cast.

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

I believe static_cast would be better style, but it all does the same thing.
down_cast just crashes earlier if it's the wrong type :P.

+  }
 
   DISALLOW_COPY_AND_ASSIGN(TestConnection);
 };
@@ -602,6 +604,16 @@ class FecQuicConnectionDebugVisitor
   QuicPacketHeader revived_header_;
 };
 
+class MockPacketWriterFactory : public QuicConnection::PacketWriterFactory {
+ public:
+  MockPacketWriterFactory(QuicPacketWriter* writer) {
+    ON_CALL(*this, Create(_)).WillByDefault(Return(writer));
+  }
+  virtual ~MockPacketWriterFactory() {}
+
+  MOCK_CONST_METHOD1(Create, QuicPacketWriter*(QuicConnection* connection));
+};
+
 class QuicConnectionTest : public ::testing::TestWithParam<QuicVersion> {
  protected:
   QuicConnectionTest()
@@ -612,8 +624,9 @@ class QuicConnectionTest : public ::testing::TestWithParam<QuicVersion> {
         loss_algorithm_(new MockLossAlgorithm()),
         helper_(new TestConnectionHelper(&clock_, &random_generator_)),
         writer_(new TestPacketWriter(version())),
+        factory_(writer_.get()),
         connection_(connection_id_, IPEndPoint(), helper_.get(),
-                    writer_.get(), false, version()),
+                    factory_, false, version()),
         frame1_(1, false, 0, MakeIOVector(data1)),
         frame2_(1, false, 3, MakeIOVector(data2)),
         sequence_number_length_(PACKET_6BYTE_SEQUENCE_NUMBER),
@@ -968,6 +981,7 @@ class QuicConnectionTest : public ::testing::TestWithParam<QuicVersion> {
   MockRandom random_generator_;
   scoped_ptr<TestConnectionHelper> helper_;
   scoped_ptr<TestPacketWriter> writer_;
+  NiceMock<MockPacketWriterFactory> factory_;
   TestConnection connection_;
   StrictMock<MockConnectionVisitor> visitor_;
 
@@ -3959,9 +3973,9 @@ TEST_P(QuicConnectionTest, Pacing) {
   ValueRestore<bool> old_flag(&FLAGS_enable_quic_pacing, true);
 
   TestConnection server(connection_id_, IPEndPoint(), helper_.get(),
-                        writer_.get(), true, version());
+                        factory_, /* is_server= */ true, version());
   TestConnection client(connection_id_, IPEndPoint(), helper_.get(),
-                        writer_.get(), false, version());
+                        factory_, /* is_server= */ false, version());
   EXPECT_TRUE(client.sent_packet_manager().using_pacing());
   EXPECT_FALSE(server.sent_packet_manager().using_pacing());
 }