Cleanup style nits in SSLClientSocketTest

net/socket/ssl_client_socket_unittest.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/ssl_client_socket.h"
 
 #include "net/base/address_list.h"
 #include "net/base/host_resolver.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_log.h"
@@ skipping 48 matching lines @@
   TestCompletionCallback callback;
   net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, &log, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   EXPECT_FALSE(sock->IsConnected());
 
   rv = sock->Connect(&callback);
   EXPECT_TRUE(net::LogContainsBeginEvent(
       log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-    EXPECT_FALSE(sock->IsConnected());
-    EXPECT_FALSE(net::LogContainsEndEvent(
-        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
-
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
-
+  EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(sock->IsConnected());
   EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
 
   sock->Disconnect();
   EXPECT_FALSE(sock->IsConnected());
 }
 
 TEST_F(SSLClientSocketTest, ConnectExpired) {
   net::TestServer::HTTPSOptions https_options(
       net::TestServer::HTTPSOptions::CERT_EXPIRED);
   net::TestServer test_server(https_options, FilePath());
   ASSERT_TRUE(test_server.Start());
 
   net::AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
   net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, &log, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   EXPECT_FALSE(sock->IsConnected());
 
   rv = sock->Connect(&callback);
   EXPECT_TRUE(net::LogContainsBeginEvent(
       log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-    EXPECT_FALSE(sock->IsConnected());
-    EXPECT_FALSE(net::LogContainsEndEvent(
-        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(net::ERR_CERT_DATE_INVALID, rv);
-  }
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, rv);
 
-  // We cannot test sock->IsConnected(), as the NSS implementation disconnects
-  // the socket when it encounters an error, whereas other implementations
-  // leave it connected.
+  // Rather than testing whether or not the underlying socket, test that the

[wtc, 2010/11/10 01:05:50]

Nit: "whether or not the underlying socket" is incomplete.
I guess it's missing "is connected" at the end?

+  // handshake has finished. This is because it may be desirable to
+  // disconnect the socket before showing a user prompt, since the user may
+  // take indefinitely long to respond.
   EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
 }
 
 TEST_F(SSLClientSocketTest, ConnectMismatched) {
   net::TestServer::HTTPSOptions https_options(
       net::TestServer::HTTPSOptions::CERT_MISMATCHED_NAME);
   net::TestServer test_server(https_options, FilePath());
   ASSERT_TRUE(test_server.Start());
 
   net::AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
   net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, &log, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   EXPECT_FALSE(sock->IsConnected());
 
   rv = sock->Connect(&callback);
 
   EXPECT_TRUE(net::LogContainsBeginEvent(
       log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
-  if (rv != net::ERR_CERT_COMMON_NAME_INVALID) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-    EXPECT_FALSE(sock->IsConnected());

[wtc, 2010/11/10 01:05:50]

On second thought, I think these two EXPECT statements (lines
170-171) are correct because of the way our message-driven IO
works -- after the sock->Connect() call on line 164 returns,
we have not given the current message loop a chance to process
more messages/run any tasks, so the state of 'sock' cannot
possibly change, and no new events will have been written to
the log.

But this point is not obvious, and is not important, so
I don't think we should add these two EXPECT statements back.

-    EXPECT_FALSE(net::LogContainsEndEvent(
-        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, rv);
-  }
+  EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, rv);
 
-  // We cannot test sock->IsConnected(), as the NSS implementation disconnects
-  // the socket when it encounters an error, whereas other implementations
-  // leave it connected.
+  // Rather than testing whether or not the underlying socket, test that the
+  // handshake has finished. This is because it may be desirable to
+  // disconnect the socket before showing a user prompt, since the user may
+  // take indefinitely long to respond.
   EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
 }
 
 // Attempt to connect to a page which requests a client certificate. It should
 // return an error code on connect.
 // Flaky: http://crbug.com/54445
 TEST_F(SSLClientSocketTest, FLAKY_ConnectClientAuthCertRequested) {
   net::TestServer::HTTPSOptions https_options;
   https_options.request_client_certificate = true;
   net::TestServer test_server(https_options, FilePath());
   ASSERT_TRUE(test_server.Start());
 
   net::AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
   net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, &log, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   EXPECT_FALSE(sock->IsConnected());
 
   rv = sock->Connect(&callback);
   EXPECT_TRUE(net::LogContainsBeginEvent(
       log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
-  if (rv != net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-    EXPECT_FALSE(sock->IsConnected());
-    EXPECT_FALSE(net::LogContainsEndEvent(
-        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, rv);
-  }
-
-  // We cannot test sock->IsConnected(), as the NSS implementation disconnects
-  // the socket when it encounters an error, whereas other implementations
-  // leave it connected.
-  EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
+  EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, rv);
+  EXPECT_FALSE(sock->IsConnected());
 }
 
 // Connect to a server requesting optional client authentication. Send it a
 // null certificate. It should allow the connection.
 //
 // TODO(davidben): Also test providing an actual certificate.
 TEST_F(SSLClientSocketTest, ConnectClientAuthSendNullCert) {
   net::TestServer::HTTPSOptions https_options;
   https_options.request_client_certificate = true;
   net::TestServer test_server(https_options, FilePath());
   ASSERT_TRUE(test_server.Start());
 
   net::AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
   net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, &log, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   net::SSLConfig ssl_config = kDefaultSSLConfig;
   ssl_config.send_client_cert = true;
   ssl_config.client_cert = NULL;
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), ssl_config,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), ssl_config,
+          NULL));
 
   EXPECT_FALSE(sock->IsConnected());
 
   // Our test server accepts certificate-less connections.
   // TODO(davidben): Add a test which requires them and verify the error.
   rv = sock->Connect(&callback);
   EXPECT_TRUE(net::LogContainsBeginEvent(
       log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-    EXPECT_FALSE(sock->IsConnected());
-    EXPECT_FALSE(net::LogContainsEndEvent(
-        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
-
+  EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(sock->IsConnected());
   EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
 
   sock->Disconnect();
   EXPECT_FALSE(sock->IsConnected());
 }
 
 // TODO(wtc): Add unit tests for IsConnectedAndIdle:
 //   - Server closes an SSL connection (with a close_notify alert message).
 //   - Server closes the underlying TCP connection directly.
 //   - Server sends data unexpectedly.
 
 TEST_F(SSLClientSocketTest, Read) {
   net::TestServer test_server(net::TestServer::TYPE_HTTPS, FilePath());
   ASSERT_TRUE(test_server.Start());
 
   net::AddressList addr;
   ASSERT_TRUE(test_server.GetAddressList(&addr));
 
   TestCompletionCallback callback;
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, NULL, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
       socket_factory_->CreateSSLClientSocket(
-          transport,
-          test_server.host_port_pair().host(),
-          kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   rv = sock->Connect(&callback);
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
+  EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(sock->IsConnected());
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   scoped_refptr<net::IOBuffer> request_buffer(
       new net::IOBuffer(arraysize(request_text) - 1));
   memcpy(request_buffer->data(), request_text, arraysize(request_text) - 1);
 
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
@@ skipping 29 matching lines @@
 
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, NULL, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
       socket_factory_->CreateSSLClientSocket(
-          transport,
-          test_server.host_port_pair().host(),
-          kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   rv = sock->Connect(&callback);
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-
+  if (rv != net::OK)

[wtc, 2010/11/10 01:05:50]

BUG: this should say
  if (rv == net::ERR_IO_PENDING)

     rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
+  EXPECT_EQ(net::OK, rv);
   EXPECT_TRUE(sock->IsConnected());
 
   // Issue a "hanging" Read first.
   scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(4096));
   rv = sock->Read(buf, 4096, &callback);
   // We haven't written the request, so there should be no response yet.
   ASSERT_EQ(net::ERR_IO_PENDING, rv);
 
   // Write the request.
   // The request is padded with a User-Agent header to a size that causes the
@@ skipping 27 matching lines @@
 
   TestCompletionCallback callback;
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, NULL, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   rv = sock->Connect(&callback);
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
+  EXPECT_EQ(net::OK, rv);
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   scoped_refptr<net::IOBuffer> request_buffer(
       new net::IOBuffer(arraysize(request_text) - 1));
   memcpy(request_buffer->data(), request_text, arraysize(request_text) - 1);
 
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING)
@@ skipping 23 matching lines @@
 
   TestCompletionCallback callback;
   net::ClientSocket* transport = new net::TCPClientSocket(
       addr, NULL, net::NetLog::Source());
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
-      socket_factory_->CreateSSLClientSocket(transport,
-          test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
+          NULL));
 
   rv = sock->Connect(&callback);
-  if (rv != net::OK) {
-    ASSERT_EQ(net::ERR_IO_PENDING, rv);
-
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(net::OK, rv);
-  }
+  EXPECT_EQ(net::OK, rv);
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   scoped_refptr<net::IOBuffer> request_buffer(
       new net::IOBuffer(arraysize(request_text) - 1));
   memcpy(request_buffer->data(), request_text, arraysize(request_text) - 1);
 
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING)
@@ skipping 45 matching lines @@
   net::ClientSocket* transport =
       new net::MockTCPClientSocket(addr, NULL, &data);
   int rv = transport->Connect(&callback);
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(net::OK, rv);
 
   scoped_ptr<net::SSLClientSocket> sock(
       socket_factory_->CreateSSLClientSocket(
           transport, test_server.host_port_pair().host(), kDefaultSSLConfig,
-          NULL /* ssl_host_info */));
+          NULL));
 
   rv = sock->Connect(&callback);
   EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
 }