Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle

net/quic/chromium/quic_http_stream_test.cc

Index: net/quic/chromium/quic_http_stream_test.cc
diff --git a/net/quic/chromium/quic_http_stream_test.cc b/net/quic/chromium/quic_http_stream_test.cc
index ff43b73adc0bb46dc6fd9662594eec77388c44a4..2412493c24581d9d0845bc89eba7f4f9b827bed5 100644
--- a/net/quic/chromium/quic_http_stream_test.cc
+++ b/net/quic/chromium/quic_http_stream_test.cc
@@ -107,11 +107,6 @@ class AutoClosingStream : public QuicHttpStream {
       HttpServerProperties* http_server_properties)
       : QuicHttpStream(std::move(session), http_server_properties) {}
 
-  void OnInitialHeadersAvailable(const SpdyHeaderBlock& headers,
-                                 size_t frame_len) override {
-    Close(false);
-  }
-
   void OnTrailingHeadersAvailable(const SpdyHeaderBlock& headers,
                                   size_t frame_len) override {
     Close(false);
@@ -185,6 +180,9 @@ class QuicHttpStreamPeer {
 };
 
 class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
+ public:
+  void CloseStream(QuicHttpStream* stream, int /*rv*/) { stream->Close(false); }
+
  protected:
   static const bool kFin = true;
   static const bool kIncludeVersion = true;
@@ -1159,10 +1157,8 @@ TEST_P(QuicHttpStreamTest, SendPostRequest) {
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously.
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
@@ -1232,10 +1228,8 @@ TEST_P(QuicHttpStreamTest, SendChunkedPostRequest) {
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
@@ -1305,10 +1299,8 @@ TEST_P(QuicHttpStreamTest, SendChunkedPostRequestWithFinalEmptyDataPacket) {
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
@@ -1373,10 +1365,8 @@ TEST_P(QuicHttpStreamTest, SendChunkedPostRequestWithOneEmptyDataPacket) {
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
@@ -1427,7 +1417,9 @@ TEST_P(QuicHttpStreamTest, DestroyedEarly) {
 
   // Ack the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
+  EXPECT_THAT(stream_->ReadResponseHeaders(

[xunjieli, 2017/05/10 19:46:58]

optional:
|stream_| is an AutoClosingStream().
Instead of making a QuicHttpStreamTest::CloseStream method, we could just bind
OnInitialHeadersAvailable method.

[Ryan Hamilton, 2017/05/10 19:57:55]

OnInitialHeadersAvailable is a method on the handle, but is no longer a method
of the QuicHttpStream (of which AutoClosingStream is a subclass). Since this is
a test of QuicHttpStream, if I hooked into the Handle's
OnInitialHeadersAvailable, it wouldn't be testing the right code path, I don't
think. (We need QuicHttpStream::Close() to run in this test). 

The old version of this test did what you proposed, but that worked because
OnInitialHeadersAvailable used to be a method of QuicHttpStream (via
QuicChromiumClientStream::Delegate). But that method is being removed in this
CL.

One we've fully async'd the Handle, we'll be able to completely nuke
AutoClosingStream, I hope.

Sound plausible?

[xunjieli, 2017/05/10 21:10:09]

Acknowledged. SGTM!

+                  base::Bind(&QuicHttpStreamTest::CloseStream,
+                             base::Unretained(this), stream_.get())),
               IsError(ERR_IO_PENDING));
 
   // Send the response with a body.
@@ -1444,7 +1436,7 @@ TEST_P(QuicHttpStreamTest, DestroyedEarly) {
   EXPECT_EQ(static_cast<int64_t>(spdy_request_headers_frame_length),
             stream_->GetTotalSentBytes());
   // Zero since the stream is closed before processing the headers.

[xunjieli, 2017/05/10 19:46:58]

nit: Let's update the comment here.

[Ryan Hamilton, 2017/05/10 19:57:55]

Done.

-  EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
+  EXPECT_EQ(31, stream_->GetTotalReceivedBytes());

[xunjieli, 2017/05/10 19:46:58]

nit: static_cast<int64_t>(response_size)

[Ryan Hamilton, 2017/05/10 19:57:55]

Done.

 }
 
 TEST_P(QuicHttpStreamTest, Priority) {
@@ -1455,7 +1447,6 @@ TEST_P(QuicHttpStreamTest, Priority) {
   AddWrite(InnerConstructRequestHeadersPacket(
       2, GetNthClientInitiatedStreamId(0), kIncludeVersion, kFin, MEDIUM,
       &spdy_request_headers_frame_length, &header_stream_offset));
-  AddWrite(ConstructAckAndRstStreamPacket(3));
   use_closing_stream_ = true;
   Initialize();
 
@@ -1485,10 +1476,10 @@ TEST_P(QuicHttpStreamTest, Priority) {
 
   // Send the response with a body.
   SetResponse("404 OK", "hello world!");
-  // In the course of processing this packet, the QuicHttpStream close itself.
-  ProcessPacket(ConstructResponseHeadersPacket(2, kFin, nullptr));
+  size_t response_size = 0;
+  ProcessPacket(ConstructResponseHeadersPacket(2, kFin, &response_size));
 
-  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(OK, callback_.WaitForResult());
 
   EXPECT_TRUE(AtEof());
 
@@ -1496,8 +1487,8 @@ TEST_P(QuicHttpStreamTest, Priority) {
   // headers and payload.
   EXPECT_EQ(static_cast<int64_t>(spdy_request_headers_frame_length),
             stream_->GetTotalSentBytes());
-  // Zero since the stream is closed before processing the headers.
-  EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
+  EXPECT_EQ(static_cast<int64_t>(response_size),
+            stream_->GetTotalReceivedBytes());
 }
 
 // Regression test for http://crbug.com/294870