This CL is a follow up to https://codereview.chromium.org/416683002/.

It updates several comments and renames member variables to increase clarity and
better match the style guide.

R=wtc@chromium.org

Review URL: https://codereview.chromium.org/448293002

Cr-Commit-Position: refs/heads/master@{#288282}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288282 0039d316-1c4b-4281-b951-d872f2087c98

net/socket/ssl_client_socket_openssl.cc

@@ -348,7 +348,7 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       transport_read_error_(OK),
       transport_write_error_(OK),
       server_cert_chain_(new PeerCertificateChain(NULL)),
-      completed_handshake_(false),
+      completed_connect_(false),
       was_ever_used_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
@@ -363,7 +363,7 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_xtn_negotiated_(false),
-      ran_handshake_finished_callback_(false),
+      handshake_succeeded_(false),
       marked_session_as_good_(false),
       net_log_(transport_->socket()->NetLog()) {
 }
@@ -493,7 +493,7 @@ void SSLClientSocketOpenSSL::Disconnect() {
   transport_write_error_ = OK;
 
   server_cert_verify_result_.Reset();
-  completed_handshake_ = false;
+  completed_connect_ = false;
 
   cert_authorities_.clear();
   cert_key_types_.clear();
@@ -508,7 +508,7 @@ void SSLClientSocketOpenSSL::Disconnect() {
 
 bool SSLClientSocketOpenSSL::IsConnected() const {
   // If the handshake has not yet completed.
-  if (!completed_handshake_)
+  if (!completed_connect_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
@@ -519,7 +519,7 @@ bool SSLClientSocketOpenSSL::IsConnected() const {
 
 bool SSLClientSocketOpenSSL::IsConnectedAndIdle() const {
   // If the handshake has not yet completed.
-  if (!completed_handshake_)
+  if (!completed_connect_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
@@ -679,18 +679,6 @@ int SSLClientSocketOpenSSL::SetSendBufferSize(int32 size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
-// static
-void SSLClientSocketOpenSSL::InfoCallback(const SSL* ssl,
-                                          int result,
-                                          int /*unused*/) {
-  SSLClientSocketOpenSSL* ssl_socket =
-      SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);
-  if (result == SSL_CB_HANDSHAKE_DONE) {
-    ssl_socket->ran_handshake_finished_callback_ = true;
-    ssl_socket->CheckIfHandshakeFinished();
-  }
-}
-
 int SSLClientSocketOpenSSL::Init() {
   DCHECK(!ssl_);
   DCHECK(!transport_bio_);
@@ -719,7 +707,7 @@ int SSLClientSocketOpenSSL::Init() {
   DCHECK(transport_bio_);
 
   // Install a callback on OpenSSL's end to plumb transport errors through.
-  BIO_set_callback(ssl_bio, &SSLClientSocketOpenSSL::BIOCallback);
+  BIO_set_callback(ssl_bio, BIOCallback);
   BIO_set_callback_arg(ssl_bio, reinterpret_cast<char*>(this));
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
@@ -1046,7 +1034,7 @@ int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
              << " (" << result << ")";
   }
 
-  completed_handshake_ = true;
+  completed_connect_ = true;
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
@@ -1486,7 +1474,7 @@ int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
 }
 
 int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
-  if (!completed_handshake_) {
+  if (!completed_connect_) {
     // If the first handshake hasn't completed then we accept any certificates
     // because we verify after the handshake.
     return 1;
@@ -1586,19 +1574,6 @@ long SSLClientSocketOpenSSL::MaybeReplayTransportError(
   return retvalue;
 }
 
-// Determines if the session for |ssl_| is in the cache, and calls the
-// handshake completion callback if that is the case.
-//
-// CheckIfHandshakeFinished is called twice per connection: once after
-// MarkSSLSessionAsGood, when the certificate has been verified, and
-// once via an OpenSSL callback when the handshake has completed. On the
-// second call, when the certificate has been verified and the handshake
-// has completed, the connection's handshake completion callback is run.
-void SSLClientSocketOpenSSL::CheckIfHandshakeFinished() {
-  if (ran_handshake_finished_callback_ && marked_session_as_good_)
-    OnHandshakeCompletion();
-}
-
 // static
 long SSLClientSocketOpenSSL::BIOCallback(
     BIO *bio,
@@ -1612,6 +1587,32 @@ long SSLClientSocketOpenSSL::BIOCallback(
       bio, cmd, argp, argi, argl, retvalue);
 }
 
+// static
+void SSLClientSocketOpenSSL::InfoCallback(const SSL* ssl,
+                                          int type,
+                                          int /*val*/) {
+  if (type == SSL_CB_HANDSHAKE_DONE) {
+    SSLClientSocketOpenSSL* ssl_socket =
+        SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);
+    ssl_socket->handshake_succeeded_ = true;
+    ssl_socket->CheckIfHandshakeFinished();
+  }
+}
+
+// Determines if both the handshake and certificate verification have completed
+// successfully, and calls the handshake completion callback if that is the
+// case.
+//
+// CheckIfHandshakeFinished is called twice per connection: once after
+// MarkSSLSessionAsGood, when the certificate has been verified, and
+// once via an OpenSSL callback when the handshake has completed. On the
+// second call, when the certificate has been verified and the handshake
+// has completed, the connection's handshake completion callback is run.
+void SSLClientSocketOpenSSL::CheckIfHandshakeFinished() {
+  if (handshake_succeeded_ && marked_session_as_good_)
+    OnHandshakeCompletion();
+}
+
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;

net/socket/ssl_client_socket_openssl.h

@@ -106,10 +106,6 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   friend class SSLClientSocket;
   friend class SSLContext;
 
-  // Callback that is run by OpenSSL to obtain information about the
-  // state of the SSL handshake.
-  static void InfoCallback(const SSL* ssl, int result, int unused);
-
   int Init();
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
@@ -172,6 +168,10 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
                           const char *argp, int argi, long argl,
                           long retvalue);
 
+  // Callback that is used to obtain information about the state of the SSL
+  // handshake.
+  static void InfoCallback(const SSL* ssl, int type, int val);
+
   void CheckIfHandshakeFinished();
 
   bool transport_send_busy_;
@@ -211,11 +211,11 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // error writing to the transport socket. A value of OK indicates no error.
   int transport_write_error_;
 
-  // Set when handshake finishes.
+  // Set when Connect finishes.
   scoped_ptr<PeerCertificateChain> server_cert_chain_;
   scoped_refptr<X509Certificate> server_cert_;
   CertVerifyResult server_cert_verify_result_;
-  bool completed_handshake_;
+  bool completed_connect_;
 
   // Set when Read() or Write() successfully reads or writes data to or from the
   // network.
@@ -275,9 +275,9 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
   // True if InfoCallback has been run with result = SSL_CB_HANDSHAKE_DONE.
-  bool ran_handshake_finished_callback_;
+  bool handshake_succeeded_;
   // True if MarkSSLSessionAsGood has been called for this socket's
-  // connection's SSL session.
+  // SSL session.
   bool marked_session_as_good_;
   // The request handle for |channel_id_service_|.
   ChannelIDService::RequestHandle channel_id_request_handle_;

net/socket/ssl_client_socket_unittest.cc

@@ -2694,7 +2694,6 @@ TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithFailure) {
 // Tests that the completion callback is run when an SSL connection
 // completes successfully.
 TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithSuccess) {
-  SpawnedTestServer::SSLOptions ssl_options;
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
@@ -2728,8 +2727,8 @@ TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithSuccess) {
   EXPECT_TRUE(ran_handshake_completion_callback_);
 }
 
-// Tests that the completion callback is run with connections
-// that do not cache their session.
+// Tests that the completion callback is run with a server that doesn't cache
+// sessions.
 TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithDisabledSessionCache) {
   SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.disable_session_cache = true;

net/socket/ssl_session_cache_openssl.cc

@@ -10,7 +10,6 @@
 #include <openssl/rand.h>
 #include <openssl/ssl.h>
 
-#include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"

net/socket/ssl_session_cache_openssl.h

@@ -8,7 +8,6 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/callback_forward.h"
 #include "net/base/net_export.h"
 
 // Avoid including OpenSSL headers here.

net/test/spawned_test_server/base_test_server.h

@@ -205,7 +205,8 @@ class BaseTestServer {
     bool enable_npn;
 
     // Whether to disable TLS session caching. When session caching is
-    // disabled, the server will generate a new Session ID for every connection.
+    // disabled, the server will use an empty session ID in the
+    // ServerHello.
     bool disable_session_cache;
   };