Cleanup of SSLHostStateDelegate and related code

chrome/browser/ssl/chrome_ssl_host_state_delegate.h

 // Copyright 2014 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.
 
 #ifndef CHROME_BROWSER_SSL_CHROME_SSL_HOST_STATE_DELEGATE_H_
 #define CHROME_BROWSER_SSL_CHROME_SSL_HOST_STATE_DELEGATE_H_
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "content/public/browser/ssl_host_state_delegate.h"
 
 class Profile;
 
 namespace base {
 class Clock;
 class DictionaryValue;
 }  //  namespace base
 
 // Implementation of the tracking of user decisions on SSL errors for sites.
 // Tracks if the user has allowed, denied, or not seen an exception for the
 // specified site, SSL fingerprint, and error. If the user makes a decision,
 // stores the decision until either the session ends or for a length of time
 // (across session restarts), based on command line flags.
 class ChromeSSLHostStateDelegate : public content::SSLHostStateDelegate {
  public:
   explicit ChromeSSLHostStateDelegate(Profile* profile);

[willchan no longer on Chromium, 2014/08/20 22:17:21]

Not for readability: in general you should prefer not to pass around Profile*
objects. Generally speaking, you should prefer to pass finer grained
dependencies, rather than huge objects like Profile which have all these other
dependencies. That will enable easier unit testing, componentization (if this
ever needs to moved to content/ or components/), etc.

[jww, 2014/08/22 00:54:17]

In this case, we need some pretty beefy stuff from Profile. Namely, we call
GetHostContentSettingsMap() several times, and we also call GetRequestContext()
on the Profile.

Is your suggestion that we should keep around raw pointers to those instead? I
haven't seen anything like that, so I sort of assumed it was healthier to grab
them off the Profile whenever they were needed (profile.h doesn't mention any
guarantees about those pointers not going stale).

   virtual ~ChromeSSLHostStateDelegate();
 
   // SSLHostStateDelegate:
   virtual void DenyCert(const std::string& host,
                         net::X509Certificate* cert,
                         net::CertStatus error) OVERRIDE;
   virtual void AllowCert(const std::string& host,
                          net::X509Certificate* cert,
                          net::CertStatus error) OVERRIDE;
   virtual void Clear() OVERRIDE;
@@ skipping 10 matching lines @@
   // ChromeSSLHostStateDelegate implementation:
   // Revoke all user decisions for |host| in the given Profile. The
   // RevokeUserDecisionsHard version may close idle connections in the process.
   // This version should be used *only* for rare events, such as a user
   // controlled button, as it may be very disruptive to the networking stack.
   virtual void RevokeUserDecisions(const std::string& host);
   virtual void RevokeUserDecisionsHard(const std::string& host);
 
   // Returns true if any decisions has been recorded for |host| for the given
   // Profile, otherwise false.
   virtual bool HasUserDecision(const std::string& host);

[willchan no longer on Chromium, 2014/08/20 22:17:21]

This member function reads like it should be const. Is there any reason it
isn't?

[jww, 2014/08/22 00:54:17]

Nope, done.

 
   // Called on the UI thread when the profile is about to be destroyed.
   void ShutdownOnUIThread() {}

[willchan no longer on Chromium, 2014/08/20 22:17:21]

Nit: What's up with this? Empty function? Does anyone call this?

[jww, 2014/08/22 00:54:17]

That was leftover from an initial implementation and is called in
chrome_ssl_host_state_delegate_factory.cc. Definitely not needed now, though.
Removed.

 
  protected:
   // SetClock takes ownership of the passed in clock.
   void SetClock(scoped_ptr<base::Clock> clock);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(ForgetInstantlySSLHostStateDelegateTest,
                            MakeAndForgetException);
   FRIEND_TEST_ALL_PREFIXES(RememberSSLHostStateDelegateTest, AfterRestart);
   FRIEND_TEST_ALL_PREFIXES(RememberSSLHostStateDelegateTest,
                            QueryPolicyExpired);
 
   // 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,

[willchan no longer on Chromium, 2014/08/20 22:17:22]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...:
"Enumerators should be named either like constants or like macros: either
kEnumName or ENUM_NAME."

Chromium's style guide says
(http://www.chromium.org/developers/coding-style#Naming): "Though the Google C++
Style Guide now says to use kConstantNaming for enums, Chromium was written
using MACRO_STYLE naming.  Continue to use this style for consistency."

Therefore, you should use ENUM_NAME.

[jww, 2014/08/22 00:54:17]

Done.

     DoNotCreateDictionaryEntries
   };
 
   // Specifies whether user SSL error decisions should be forgetten at the end
   // of this current session (the old style of remembering decisions), or
   // whether they should be remembered across session restarts for a specified
   // length of time, deteremined by
   // |default_ssl_cert_decision_expiration_delta_|.
   enum RememberSSLExceptionDecisionsDisposition {
     ForgetSSLExceptionDecisionsAtSessionEnd,
     RememberSSLExceptionDecisionsForDelta
   };
 
   // Modify the user's content settings to specify a judgement made for a
   // specific site and certificate, where |url| is the site in question, |cert|
   // 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,

[willchan no longer on Chromium, 2014/08/20 22:17:21]

Is there a reason |cert| isn't a const reference? Do you modify it at all?

I'd make the same comment in the member functions above, except those appear to
be virtuals inherited from an interface out of your control here.

[jww, 2014/08/22 00:54:17]

Nope, those can all be const. And, in fact, the interface is within my control
:-) It was part of the original CL, but it seemed like overload to include it as
part of the readability submission, but I'll add it in now.

                         net::CertStatus error,
                         net::CertPolicy::Judgment judgment);
 
   // Query the content settings to retrieve a dictionary of certificate
   // fingerprints and errors of certificates to user decisions, as set by
   // ChangeCertPolicy. Returns NULL on a failure.
   //
   // |dict| specifies the user's full exceptions dictionary for a specific site
   // in their content settings. Must be retrieved directly from a website
   // setting in the the profile's HostContentSettingsMap.
@@ skipping 15 matching lines @@
   base::TimeDelta default_ssl_cert_decision_expiration_delta_;
   Profile* profile_;
 
   // A BrokenHostEntry is a pair of (host, process_id) that indicates the host
   // contains insecure content in that renderer process.
   typedef std::pair<std::string, int> BrokenHostEntry;
 
   // Hosts which have been contaminated with insecure content in the
   // specified process.  Note that insecure content can travel between
   // same-origin frames in one processs but cannot jump between processes.
   std::set<BrokenHostEntry> ran_insecure_content_hosts_;

[willchan no longer on Chromium, 2014/08/20 22:17:21]

You should #include <set>.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...:

"""
Always #include the file that actually provides the declarations/definitions you
need; do not rely on the symbol being brought in transitively via headers not
directly included. One exception is that myfile.cc may rely on #includes and
forward declarations from its corresponding header file myfile.h.
"""

[jww, 2014/08/22 00:54:17]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(ChromeSSLHostStateDelegate);
 };
 
 #endif  // CHROME_BROWSER_SSL_CHROME_SSL_HOST_STATE_DELEGATE_H_
+