spdy_test_util_common.{h,cc} cleanup.

* Remove duplicate includes.
* Include spdy_session.h for SpdySession::TimeFunc.
* s/NULL/nullptr/
* Ever since https://crrev.com/199633019 landed three and a half years
  ago, |expected_status| argument of CreateSpdySessionHelper() is
  unused.  Remove it, and remove the inaccurately named
  TryCreateSpdySessionExpectingFailure().
* Move |this_header| and |this_value| declaration closest to their first
  use in AppendToHeaderBlock().  The compiler should be smart enough to
  optimize this.
* In the same method use implicit pointer to bool conversion in DCHECKs.
* Purge naked news in favor of MakeRefCounted<> or make_unique<>.
* s/expected_error/expected_status/ for consistency.
* Use TestCompletionCallback::GetResult() [1] in
  CreateSpdySessionHelper() instead of inlining it.

[1] https://cs.chromium.org/chromium/src/net/base/test_completion_callback.h?l=72

Bug: 
Change-Id: Id5bc3f37f068f78901afbf8fb027cc6c63744b22
Reviewed-on: https://chromium-review.googlesource.com/775868
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517847}

net/spdy/chromium/spdy_network_transaction_unittest.cc

@@ -4065,6 +4065,28 @@ TEST_F(SpdyNetworkTransactionTest, CloseWithActiveStream) {
   helper.VerifyDataConsumed();
 }
 
+TEST_F(SpdyNetworkTransactionTest, GoAwayImmediately) {
+  SpdySerializedFrame goaway(spdy_util_.ConstructSpdyGoAway(1));
+  MockRead reads[] = {CreateMockRead(goaway, 0, SYNCHRONOUS)};
+  SequencedSocketData data(reads, arraysize(reads), nullptr, 0);
+
+  NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_, nullptr);
+  helper.RunPreTestSetup();
+  helper.AddData(&data);
+  helper.StartDefaultTest();
+  EXPECT_THAT(helper.output().rv, IsError(ERR_IO_PENDING));
+
+  helper.WaitForCallbackToComplete();
+  EXPECT_THAT(helper.output().rv, IsError(ERR_CONNECTION_CLOSED));
+
+  const HttpResponseInfo* response = helper.trans()->GetResponseInfo();
+  EXPECT_FALSE(response->headers);
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+
+  // Verify that we consumed all test data.
+  helper.VerifyDataConsumed();
+}
+
 // Retry with HTTP/1.1 when receiving HTTP_1_1_REQUIRED.  Note that no actual
 // protocol negotiation happens, instead this test forces protocols for both
 // sockets.

net/spdy/chromium/spdy_session_unittest.cc

@@ -347,9 +347,7 @@ TEST_F(SpdySessionTest, GoAwayImmediatelyWithNoActiveStreams) {
   AddSSLSocketData();
 
   CreateNetworkSession();
-
-  session_ = TryCreateSpdySessionExpectingFailure(
-      http_session_.get(), key_, ERR_CONNECTION_CLOSED, NetLogWithSource());
+  CreateSpdySession();
   base::RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(session_);

net/spdy/chromium/spdy_test_util_common.cc

@@ -4,8 +4,6 @@
 
 #include "net/spdy/chromium/spdy_test_util_common.h"
 
-#include <stdint.h>
-
 #include <cstddef>
 #include <utility>
 
@@ -20,18 +18,14 @@
 #include "net/cert/mock_cert_verifier.h"
 #include "net/cert/signed_certificate_timestamp_and_status.h"
 #include "net/http/http_cache.h"
-#include "net/http/http_network_session.h"
 #include "net/http/http_network_transaction.h"
-#include "net/http/http_server_properties_impl.h"
 #include "net/log/net_log_with_source.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/next_proto.h"
-#include "net/socket/socket_test_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/transport_client_socket_pool.h"
 #include "net/spdy/chromium/buffered_spdy_framer.h"
 #include "net/spdy/chromium/spdy_http_utils.h"
-#include "net/spdy/chromium/spdy_session.h"
 #include "net/spdy/chromium/spdy_session_pool.h"
 #include "net/spdy/chromium/spdy_stream.h"
 #include "net/spdy/core/spdy_alt_svc_wire_format.h"
@@ -40,6 +34,7 @@
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+using net::test::IsError;
 using net::test::IsOk;
 
 namespace net {
@@ -89,25 +84,23 @@ std::unique_ptr<MockWrite[]> ChopWriteFrame(const SpdySerializedFrame& frame,
 void AppendToHeaderBlock(const char* const extra_headers[],
                          int extra_header_count,
                          SpdyHeaderBlock* headers) {
-  SpdyString this_header;
-  SpdyString this_value;
-
   if (!extra_header_count)
     return;
 
   // Sanity check: Non-NULL header list.
-  DCHECK(NULL != extra_headers) << "NULL header value pair list";
+  DCHECK(extra_headers) << "NULL header value pair list";
   // Sanity check: Non-NULL header map.
-  DCHECK(NULL != headers) << "NULL header map";
+  DCHECK(headers) << "NULL header map";
+
   // Copy in the headers.
   for (int i = 0; i < extra_header_count; i++) {
     // Sanity check: Non-empty header.
     DCHECK_NE('\0', *extra_headers[i * 2]) << "Empty header value pair";
-    this_header = extra_headers[i * 2];
+    SpdyString this_header = extra_headers[i * 2];
     SpdyString::size_type header_len = this_header.length();
     if (!header_len)
       continue;
-    this_value = extra_headers[1 + (i * 2)];
+    SpdyString this_value = extra_headers[1 + (i * 2)];
     SpdyString new_value;
     if (headers->find(this_header) != headers->end()) {
       // More than one entry in the header.
@@ -329,18 +322,18 @@ SpdySessionDependencies::SpdySessionDependencies()
 
 SpdySessionDependencies::SpdySessionDependencies(
     std::unique_ptr<ProxyService> proxy_service)
-    : host_resolver(new MockCachingHostResolver),
-      cert_verifier(new MockCertVerifier),
+    : host_resolver(std::make_unique<MockCachingHostResolver>()),
+      cert_verifier(std::make_unique<MockCertVerifier>()),
       channel_id_service(nullptr),
-      transport_security_state(new TransportSecurityState),
-      cert_transparency_verifier(new DoNothingCTVerifier),
-      ct_policy_enforcer(new CTPolicyEnforcer),
+      transport_security_state(std::make_unique<TransportSecurityState>()),
+      cert_transparency_verifier(std::make_unique<DoNothingCTVerifier>()),
+      ct_policy_enforcer(std::make_unique<CTPolicyEnforcer>()),
       proxy_service(std::move(proxy_service)),
-      ssl_config_service(new SSLConfigServiceDefaults),
-      socket_factory(new MockClientSocketFactory),
+      ssl_config_service(base::MakeRefCounted<SSLConfigServiceDefaults>()),
+      socket_factory(std::make_unique<MockClientSocketFactory>()),
       http_auth_handler_factory(
           HttpAuthHandlerFactory::CreateDefault(host_resolver.get())),
-      http_server_properties(new HttpServerPropertiesImpl),
+      http_server_properties(std::make_unique<HttpServerPropertiesImpl>()),
       enable_ip_pooling(true),
       enable_ping(false),
       enable_user_alternate_protocol_ports(false),
@@ -443,8 +436,7 @@ class AllowAnyCertCTPolicyEnforcer : public CTPolicyEnforcer {
 };
 
 SpdyURLRequestContext::SpdyURLRequestContext() : storage_(this) {
-  storage_.set_host_resolver(
-      std::unique_ptr<HostResolver>(new MockHostResolver));
+  storage_.set_host_resolver(std::make_unique<MockHostResolver>());
   storage_.set_cert_verifier(std::make_unique<MockCertVerifier>());
   storage_.set_transport_security_state(
       std::make_unique<TransportSecurityState>());
@@ -457,7 +449,7 @@ SpdyURLRequestContext::SpdyURLRequestContext() : storage_(this) {
   storage_.set_http_auth_handler_factory(
       HttpAuthHandlerFactory::CreateDefault(host_resolver()));
   storage_.set_http_server_properties(
-      std::unique_ptr<HttpServerProperties>(new HttpServerPropertiesImpl()));
+      std::make_unique<HttpServerPropertiesImpl>());
   storage_.set_job_factory(std::make_unique<URLRequestJobFactoryImpl>());
   HttpNetworkSession::Params session_params;
   session_params.enable_spdy_ping_based_connection_checking = false;
@@ -498,33 +490,28 @@ base::WeakPtr<SpdySession> CreateSpdySessionHelper(
     HttpNetworkSession* http_session,
     const SpdySessionKey& key,
     const NetLogWithSource& net_log,
-    Error expected_status,
     bool enable_ip_based_pooling) {
   EXPECT_FALSE(http_session->spdy_session_pool()->FindAvailableSession(
       key, enable_ip_based_pooling, NetLogWithSource()));
 
-  scoped_refptr<TransportSocketParams> transport_params(
-      new TransportSocketParams(
-          key.host_port_pair(), false, OnHostResolutionCallback(),
-          TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT));
+  auto transport_params = base::MakeRefCounted<TransportSocketParams>(
+      key.host_port_pair(), /* disable_resolver_cache = */ false,
+      OnHostResolutionCallback(),
+      TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT);
 
   auto connection = std::make_unique<ClientSocketHandle>();
   TestCompletionCallback callback;
 
-  int rv = ERR_UNEXPECTED;
   SSLConfig ssl_config;
-  scoped_refptr<SSLSocketParams> ssl_params(
-      new SSLSocketParams(transport_params, NULL, NULL, key.host_port_pair(),
-                          ssl_config, key.privacy_mode(), 0, false));
-  rv = connection->Init(
+  auto ssl_params = base::MakeRefCounted<SSLSocketParams>(
+      transport_params, nullptr, nullptr, key.host_port_pair(), ssl_config,
+      key.privacy_mode(), 0, /* expect_spdy = */ false);
+  int rv = connection->Init(
       key.host_port_pair().ToString(), ssl_params, MEDIUM,
       ClientSocketPool::RespectLimits::ENABLED, callback.callback(),
       http_session->GetSSLSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL),
       net_log);
-
-  if (rv == ERR_IO_PENDING)
-    rv = callback.WaitForResult();
-
+  rv = callback.GetResult(rv);
   EXPECT_THAT(rv, IsOk());
 
   base::WeakPtr<SpdySession> spdy_session =
@@ -538,20 +525,10 @@ base::WeakPtr<SpdySession> CreateSpdySessionHelper(
 
 }  // namespace
 
-base::WeakPtr<SpdySession> TryCreateSpdySessionExpectingFailure(
-    HttpNetworkSession* http_session,
-    const SpdySessionKey& key,
-    Error expected_error,
-    const NetLogWithSource& net_log) {
-  DCHECK_LT(expected_error, ERR_IO_PENDING);
-  return CreateSpdySessionHelper(http_session, key, net_log, expected_error,
-                                 /* enable_ip_based_pooling = */ true);
-}
-
 base::WeakPtr<SpdySession> CreateSpdySession(HttpNetworkSession* http_session,
                                              const SpdySessionKey& key,
                                              const NetLogWithSource& net_log) {
-  return CreateSpdySessionHelper(http_session, key, net_log, OK,
+  return CreateSpdySessionHelper(http_session, key, net_log,
                                  /* enable_ip_based_pooling = */ true);
 }
 
@@ -559,7 +536,7 @@ base::WeakPtr<SpdySession> CreateSpdySessionWithIpBasedPoolingDisabled(
     HttpNetworkSession* http_session,
     const SpdySessionKey& key,
     const NetLogWithSource& net_log) {
-  return CreateSpdySessionHelper(http_session, key, net_log, OK,
+  return CreateSpdySessionHelper(http_session, key, net_log,
                                  /* enable_ip_based_pooling = */ false);
 }
 
@@ -647,9 +624,9 @@ base::WeakPtr<SpdySession> CreateFakeSpdySession(SpdySessionPool* pool,
 base::WeakPtr<SpdySession> TryCreateFakeSpdySessionExpectingFailure(
     SpdySessionPool* pool,
     const SpdySessionKey& key,
-    Error expected_error) {
-  DCHECK_LT(expected_error, ERR_IO_PENDING);
-  return CreateFakeSpdySessionHelper(pool, key, expected_error);
+    Error expected_status) {
+  DCHECK_LT(expected_status, ERR_IO_PENDING);
+  return CreateFakeSpdySessionHelper(pool, key, expected_status);
 }
 
 SpdySessionPoolPeer::SpdySessionPoolPeer(SpdySessionPool* pool) : pool_(pool) {
@@ -682,7 +659,7 @@ void SpdyTestUtil::AddUrlToHeaderBlock(SpdyStringPiece url,
 
 // static
 SpdyHeaderBlock SpdyTestUtil::ConstructGetHeaderBlock(SpdyStringPiece url) {
-  return ConstructHeaderBlock("GET", url, NULL);
+  return ConstructHeaderBlock("GET", url, nullptr);
 }
 
 // static
@@ -974,7 +951,7 @@ SpdySerializedFrame SpdyTestUtil::ConstructSpdyReplyError(
 }
 
 SpdySerializedFrame SpdyTestUtil::ConstructSpdyReplyError(int stream_id) {
-  return ConstructSpdyReplyError("500", NULL, 0, 1);
+  return ConstructSpdyReplyError("500", nullptr, 0, 1);
 }
 
 SpdySerializedFrame SpdyTestUtil::ConstructSpdyGetReply(

net/spdy/chromium/spdy_test_util_common.h

@@ -30,6 +30,7 @@
 #include "net/proxy/proxy_server.h"
 #include "net/proxy/proxy_service.h"
 #include "net/socket/socket_test_util.h"
+#include "net/spdy/chromium/spdy_session.h"
 #include "net/spdy/core/spdy_protocol.h"
 #include "net/spdy/platform/api/spdy_string.h"
 #include "net/spdy/platform/api/spdy_string_piece.h"
@@ -46,7 +47,6 @@ class CTVerifier;
 class CTPolicyEnforcer;
 class HostPortPair;
 class NetLogWithSource;
-class SpdySession;
 class SpdySessionKey;
 class SpdySessionPool;
 class SpdyStream;
@@ -238,16 +238,6 @@ class SpdyURLRequestContext : public URLRequestContext {
 // NULL.
 bool HasSpdySession(SpdySessionPool* pool, const SpdySessionKey& key);
 
-// Tries to create a SPDY session for the given key but expects the
-// attempt to fail with the given error. A SPDY session for |key| must
-// not already exist. The session will be created but close in the
-// next event loop iteration.
-base::WeakPtr<SpdySession> TryCreateSpdySessionExpectingFailure(
-    HttpNetworkSession* http_session,
-    const SpdySessionKey& key,
-    Error expected_error,
-    const NetLogWithSource& net_log);
-
 // Creates a SPDY session for the given key and puts it in the SPDY
 // session pool in |http_session|. A SPDY session for |key| must not
 // already exist.
@@ -276,7 +266,7 @@ base::WeakPtr<SpdySession> CreateFakeSpdySession(SpdySessionPool* pool,
 base::WeakPtr<SpdySession> TryCreateFakeSpdySessionExpectingFailure(
     SpdySessionPool* pool,
     const SpdySessionKey& key,
-    Error expected_error);
+    Error expected_status);
 
 class SpdySessionPoolPeer {
  public: