Cleanup of SSLHostStateDelegate and related code.

This is a general cleanup of some minor issues, mostly style and
comments, for SSLHostStateDelegate and related classes and methods. This
was done originally as a C++ readability review request, originally
based on https://codereview.chromium.org/369703002/.

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

Cr-Commit-Position: refs/heads/master@{#293443}

chrome/browser/ssl/chrome_ssl_host_state_delegate.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h"
 
+#include <set>
+
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -13,6 +15,7 @@
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
@@ -72,7 +75,7 @@ GURL GetSecureGURLForHost(const std::string& host) {
 
 // This is a helper function that returns the length of time before a
 // certificate decision expires based on the command line flags. Returns a
-// non-negative value in seconds or a value of -1  indicating that decisions
+// non-negative value in seconds or a value of -1 indicating that decisions
 // should not be remembered after the current session has ended (but should be
 // remembered indefinitely as long as the session does not end), which is the
 // "old" style of certificate decision memory. Uses the experimental group
@@ -118,12 +121,12 @@ int64 GetExpirationDelta() {
   return kForgetAtSessionEndSwitchValue;
 }
 
-std::string GetKey(net::X509Certificate* cert, net::CertStatus error) {
+std::string GetKey(const net::X509Certificate& cert, net::CertStatus error) {
   // Since a security decision will be made based on the fingerprint, Chrome
   // should use the SHA-256 fingerprint for the certificate.
   net::SHA256HashValue fingerprint =
       net::X509Certificate::CalculateChainFingerprint256(
-          cert->os_cert_handle(), cert->GetIntermediateCertificates());
+          cert.os_cert_handle(), cert.GetIntermediateCertificates());
   std::string base64_fingerprint;
   base::Base64Encode(
       base::StringPiece(reinterpret_cast<const char*>(fingerprint.data),
@@ -139,12 +142,12 @@ std::string GetKey(net::X509Certificate* cert, net::CertStatus error) {
 // dictionary that has been passed in. The returned pointer is owned by the the
 // argument dict that is passed in.
 //
-// If create_entries is set to |DoNotCreateDictionaryEntries|,
+// If create_entries is set to |DO_NOT_CREATE_DICTIONARY_ENTRIES|,
 // GetValidCertDecisionsDict will return NULL if there is anything invalid about
 // the setting, such as an invalid version or invalid value types (in addition
-// to there not be any values in the dictionary). If create_entries is set to
-// |CreateDictionaryEntries|, if no dictionary is found or the decisions are
-// expired, a new dictionary will be created
+// to there not being any values in the dictionary). If create_entries is set to
+// |CREATE_DICTIONARY_ENTRIES|, if no dictionary is found or the decisions are
+// expired, a new dictionary will be created.
 base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
     base::DictionaryValue* dict,
     CreateDictionaryEntriesDisposition create_entries,
@@ -158,7 +161,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
   int version;
   bool success = dict->GetInteger(kSSLCertDecisionVersionKey, &version);
   if (!success) {
-    if (create_entries == DoNotCreateDictionaryEntries)
+    if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES)
       return NULL;
 
     dict->SetInteger(kSSLCertDecisionVersionKey,
@@ -200,16 +203,16 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
   }
 
   // Check to see if the user's certificate decision has expired.
-  // - Expired and |create_entries| is DoNotCreateDictionaryEntries, return
+  // - Expired and |create_entries| is DO_NOT_CREATE_DICTIONARY_ENTRIES, return
   // NULL.
-  // - Expired and |create_entries| is CreateDictionaryEntries, update the
+  // - Expired and |create_entries| is CREATE_DICTIONARY_ENTRIES, update the
   // expiration time.
   if (should_remember_ssl_decisions_ !=
-          ForgetSSLExceptionDecisionsAtSessionEnd &&
+          FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END &&
       decision_expiration.ToInternalValue() <= now.ToInternalValue()) {
     *expired_previous_decision = true;
 
-    if (create_entries == DoNotCreateDictionaryEntries)
+    if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES)
       return NULL;
 
     expired = true;
@@ -226,7 +229,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
   base::DictionaryValue* cert_error_dict = NULL;  // Will be owned by dict
   if (expired ||
       !dict->GetDictionary(kSSLCertDecisionCertErrorMapKey, &cert_error_dict)) {
-    if (create_entries == DoNotCreateDictionaryEntries)
+    if (create_entries == DO_NOT_CREATE_DICTIONARY_ENTRIES)
       return NULL;
 
     cert_error_dict = new base::DictionaryValue();
@@ -238,7 +241,7 @@ base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(
 }
 
 // If |should_remember_ssl_decisions_| is
-// ForgetSSLExceptionDecisionsAtSessionEnd, that means that all invalid
+// FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END, that means that all invalid
 // certificate proceed decisions should be forgotten when the session ends. At
 // attempt is made in the destructor to remove the entries, but in the case that
 // things didn't shut down cleanly, on start, Clear is called to guarantee a
@@ -247,29 +250,31 @@ ChromeSSLHostStateDelegate::ChromeSSLHostStateDelegate(Profile* profile)
     : clock_(new base::DefaultClock()), profile_(profile) {
   int64 expiration_delta = GetExpirationDelta();
   if (expiration_delta == kForgetAtSessionEndSwitchValue) {
-    should_remember_ssl_decisions_ = ForgetSSLExceptionDecisionsAtSessionEnd;
+    should_remember_ssl_decisions_ =
+        FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END;
     expiration_delta = 0;
     Clear();
   } else {
-    should_remember_ssl_decisions_ = RememberSSLExceptionDecisionsForDelta;
+    should_remember_ssl_decisions_ = REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA;
   }
   default_ssl_cert_decision_expiration_delta_ =
       base::TimeDelta::FromSeconds(expiration_delta);
 }
 
 ChromeSSLHostStateDelegate::~ChromeSSLHostStateDelegate() {
-  if (should_remember_ssl_decisions_ == ForgetSSLExceptionDecisionsAtSessionEnd)
+  if (should_remember_ssl_decisions_ ==
+      FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END)
     Clear();
 }
 
 void ChromeSSLHostStateDelegate::DenyCert(const std::string& host,
-                                          net::X509Certificate* cert,
+                                          const net::X509Certificate& cert,
                                           net::CertStatus error) {
   ChangeCertPolicy(host, cert, error, net::CertPolicy::DENIED);
 }
 
 void ChromeSSLHostStateDelegate::AllowCert(const std::string& host,
-                                           net::X509Certificate* cert,
+                                           const net::X509Certificate& cert,
                                            net::CertStatus error) {
   ChangeCertPolicy(host, cert, error, net::CertPolicy::ALLOWED);
 }
@@ -281,7 +286,7 @@ void ChromeSSLHostStateDelegate::Clear() {
 
 net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
     const std::string& host,
-    net::X509Certificate* cert,
+    const net::X509Certificate& cert,
     net::CertStatus error,
     bool* expired_previous_decision) {
   HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
@@ -302,9 +307,9 @@ net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
 
   base::DictionaryValue* cert_error_dict;  // Owned by value
   cert_error_dict = GetValidCertDecisionsDict(
-      dict, DoNotCreateDictionaryEntries, expired_previous_decision);
+      dict, DO_NOT_CREATE_DICTIONARY_ENTRIES, expired_previous_decision);
   if (!cert_error_dict) {
-    // This revoke is necessary to clear any old expired setting that may
+    // This revoke is necessary to clear any old expired setting that may be
     // lingering in the case that an old decision expried.
     RevokeUserDecisions(host);
     return net::CertPolicy::UNKNOWN;
@@ -356,11 +361,12 @@ void ChromeSSLHostStateDelegate::RevokeUserDecisionsHard(
   RevokeUserDecisions(host);
   scoped_refptr<net::URLRequestContextGetter> getter(
       profile_->GetRequestContext());
-  profile_->GetRequestContext()->GetNetworkTaskRunner()->PostTask(
+  getter->GetNetworkTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&CloseIdleConnections, getter));
 }
 
-bool ChromeSSLHostStateDelegate::HasUserDecision(const std::string& host) {
+bool ChromeSSLHostStateDelegate::HasUserDecision(
+    const std::string& host) const {
   GURL url = GetSecureGURLForHost(host);
   const ContentSettingsPattern pattern =
       ContentSettingsPattern::FromURLNoWildcard(url);
@@ -403,9 +409,9 @@ void ChromeSSLHostStateDelegate::SetClock(scoped_ptr<base::Clock> clock) {
 
 void ChromeSSLHostStateDelegate::ChangeCertPolicy(
     const std::string& host,
-    net::X509Certificate* cert,
+    const net::X509Certificate& cert,
     net::CertStatus error,
-    net::CertPolicy::Judgment judgment) {
+    const net::CertPolicy::Judgment judgment) {
   GURL url = GetSecureGURLForHost(host);
   const ContentSettingsPattern pattern =
       ContentSettingsPattern::FromURLNoWildcard(url);
@@ -422,7 +428,7 @@ void ChromeSSLHostStateDelegate::ChangeCertPolicy(
 
   bool expired_previous_decision;  // unused value in this function
   base::DictionaryValue* cert_dict = GetValidCertDecisionsDict(
-      dict, CreateDictionaryEntries, &expired_previous_decision);
+      dict, CREATE_DICTIONARY_ENTRIES, &expired_previous_decision);
   // If a a valid certificate dictionary cannot be extracted from the content
   // setting, that means it's in an unknown format. Unfortunately, there's
   // nothing to be done in that case, so a silent fail is the only option.

chrome/browser/ssl/chrome_ssl_host_state_delegate.h

@@ -29,15 +29,15 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
 
   // SSLHostStateDelegate:
   virtual void DenyCert(const std::string& host,
-                        net::X509Certificate* cert,
+                        const net::X509Certificate& cert,
                         net::CertStatus error) OVERRIDE;
   virtual void AllowCert(const std::string& host,
-                         net::X509Certificate* cert,
+                         const net::X509Certificate& cert,
                          net::CertStatus error) OVERRIDE;
   virtual void Clear() OVERRIDE;
   virtual net::CertPolicy::Judgment QueryPolicy(
       const std::string& host,
-      net::X509Certificate* cert,
+      const net::X509Certificate& cert,
       net::CertStatus error,
       bool* expired_previous_decision) OVERRIDE;
   virtual void HostRanInsecureContent(const std::string& host,
@@ -55,10 +55,7 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
 
   // Returns true if any decisions has been recorded for |host| for the given
   // Profile, otherwise false.
-  virtual bool HasUserDecision(const std::string& host);
-
-  // Called on the UI thread when the profile is about to be destroyed.
-  void ShutdownOnUIThread() {}
+  virtual bool HasUserDecision(const std::string& host) const;
 
  protected:
   // SetClock takes ownership of the passed in clock.
@@ -74,8 +71,8 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
   // Used to specify whether new content setting entries should be created if
   // they don't already exist when querying the user's settings.
   enum CreateDictionaryEntriesDisposition {
-    CreateDictionaryEntries,
-    DoNotCreateDictionaryEntries
+    CREATE_DICTIONARY_ENTRIES,
+    DO_NOT_CREATE_DICTIONARY_ENTRIES
   };
 
   // Specifies whether user SSL error decisions should be forgetten at the end
@@ -84,8 +81,8 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
   // length of time, deteremined by
   // |default_ssl_cert_decision_expiration_delta_|.
   enum RememberSSLExceptionDecisionsDisposition {
-    ForgetSSLExceptionDecisionsAtSessionEnd,
-    RememberSSLExceptionDecisionsForDelta
+    FORGET_SSL_EXCEPTION_DECISIONS_AT_SESSION_END,
+    REMEMBER_SSL_EXCEPTION_DECISIONS_FOR_DELTA
   };
 
   // Modify the user's content settings to specify a judgement made for a
@@ -93,7 +90,7 @@ class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
   // is the certificate with an error, |error| is the error in the certificate,
   // and |judgement| is the user decision to be recorded.
   void ChangeCertPolicy(const std::string& host,
-                        net::X509Certificate* cert,
+                        const net::X509Certificate& cert,
                         net::CertStatus error,
                         net::CertPolicy::Judgment judgment);
 

chrome/browser/ssl/chrome_ssl_host_state_delegate_factory.cc

@@ -20,7 +20,7 @@ class Service : public KeyedService {
 
   ChromeSSLHostStateDelegate* decisions() { return decisions_.get(); }
 
-  virtual void Shutdown() OVERRIDE { decisions_->ShutdownOnUIThread(); }
+  virtual void Shutdown() OVERRIDE {}
 
  private:
   scoped_ptr<ChromeSSLHostStateDelegate> decisions_;

content/browser/ssl/ssl_policy.cc

@@ -34,8 +34,9 @@ SSLPolicy::SSLPolicy(SSLPolicyBackend* backend)
 void SSLPolicy::OnCertError(SSLCertErrorHandler* handler) {
   bool expired_previous_decision;
   // First we check if we know the policy for this error.
+  DCHECK(handler->ssl_info().is_valid());
   net::CertPolicy::Judgment judgment =
-      backend_->QueryPolicy(handler->ssl_info().cert.get(),
+      backend_->QueryPolicy(*handler->ssl_info().cert.get(),
                             handler->request_url().host(),
                             handler->cert_error(),
                             &expired_previous_decision);
@@ -163,6 +164,7 @@ void SSLPolicy::UpdateEntry(NavigationEntryImpl* entry,
 
 void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLCertErrorHandler> handler,
                                    bool allow) {
+  DCHECK(handler->ssl_info().is_valid());
   if (allow) {
     // Default behavior for accepting a certificate.
     // Note that we should not call SetMaxSecurityStyle here, because the active
@@ -174,7 +176,7 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLCertErrorHandler> handler,
     // While AllowCertForHost() executes synchronously on this thread,
     // ContinueRequest() gets posted to a different thread. Calling
     // AllowCertForHost() first ensures deterministic ordering.
-    backend_->AllowCertForHost(handler->ssl_info().cert.get(),
+    backend_->AllowCertForHost(*handler->ssl_info().cert.get(),
                                handler->request_url().host(),
                                handler->cert_error());
     handler->ContinueRequest();
@@ -184,7 +186,7 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLCertErrorHandler> handler,
     // While DenyCertForHost() executes synchronously on this thread,
     // CancelRequest() gets posted to a different thread. Calling
     // DenyCertForHost() first ensures deterministic ordering.
-    backend_->DenyCertForHost(handler->ssl_info().cert.get(),
+    backend_->DenyCertForHost(*handler->ssl_info().cert.get(),
                               handler->request_url().host(),
                               handler->cert_error());
     handler->CancelRequest();

content/browser/ssl/ssl_policy_backend.cc

@@ -31,14 +31,14 @@ bool SSLPolicyBackend::DidHostRunInsecureContent(const std::string& host,
   return ssl_host_state_delegate_->DidHostRunInsecureContent(host, pid);
 }
 
-void SSLPolicyBackend::DenyCertForHost(net::X509Certificate* cert,
+void SSLPolicyBackend::DenyCertForHost(const net::X509Certificate& cert,
                                        const std::string& host,
                                        net::CertStatus error) {
   if (ssl_host_state_delegate_)
     ssl_host_state_delegate_->DenyCert(host, cert, error);
 }
 
-void SSLPolicyBackend::AllowCertForHost(net::X509Certificate* cert,
+void SSLPolicyBackend::AllowCertForHost(const net::X509Certificate& cert,
                                         const std::string& host,
                                         net::CertStatus error) {
   if (ssl_host_state_delegate_)
@@ -46,7 +46,7 @@ void SSLPolicyBackend::AllowCertForHost(net::X509Certificate* cert,
 }
 
 net::CertPolicy::Judgment SSLPolicyBackend::QueryPolicy(
-    net::X509Certificate* cert,
+    const net::X509Certificate& cert,
     const std::string& host,
     net::CertStatus error,
     bool* expired_previous_decision) {

content/browser/ssl/ssl_policy_backend.h

@@ -29,20 +29,20 @@ class SSLPolicyBackend {
 
   // Records that |cert| is not permitted to be used for |host| in the future,
   // for a specific error type.
-  void DenyCertForHost(net::X509Certificate* cert,
+  void DenyCertForHost(const net::X509Certificate& cert,
                        const std::string& host,
                        net::CertStatus error);
 
   // Records that |cert| is permitted to be used for |host| in the future, for
   // a specific error type.
-  void AllowCertForHost(net::X509Certificate* cert,
+  void AllowCertForHost(const net::X509Certificate& cert,
                         const std::string& host,
                         net::CertStatus error);
 
   // Queries whether |cert| is allowed or denied for |host|. Returns true in
   // |expired_previous_decision| if a user decision had been made previously but
   // that decision has expired, otherwise false.
-  net::CertPolicy::Judgment QueryPolicy(net::X509Certificate* cert,
+  net::CertPolicy::Judgment QueryPolicy(const net::X509Certificate& cert,
                                         const std::string& host,
                                         net::CertStatus error,
                                         bool* expired_previous_decision);

content/public/browser/ssl_host_state_delegate.h

@@ -27,13 +27,13 @@ class SSLHostStateDelegate {
   // Records that |cert| is not permitted to be used for |host| in the future,
   // for a specified |error| type.
   virtual void DenyCert(const std::string& host,
-                        net::X509Certificate* cert,
+                        const net::X509Certificate& cert,
                         net::CertStatus error) = 0;
 
   // Records that |cert| is permitted to be used for |host| in the future, for
   // a specified |error| type.
   virtual void AllowCert(const std::string&,
-                         net::X509Certificate* cert,
+                         const net::X509Certificate& cert,
                          net::CertStatus error) = 0;
 
   // Clear all allow/deny preferences.
@@ -44,7 +44,7 @@ class SSLHostStateDelegate {
   // immediately prior to this query, otherwise false.
   virtual net::CertPolicy::Judgment QueryPolicy(
       const std::string& host,
-      net::X509Certificate* cert,
+      const net::X509Certificate& cert,
       net::CertStatus error,
       bool* expired_previous_decision) = 0;