Cleanup of SSLHostStateDelegate and related code

chrome/browser/ssl/chrome_ssl_host_state_delegate.cc

 // 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.
 
 #include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h"
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 54 matching lines @@
 // canonicalizes all hosts into a secure scheme GURL to use with content
 // settings. The returned GURL will be the passed in host with an empty path and
 // https:// as the scheme.
 GURL GetSecureGURLForHost(const std::string& host) {
   std::string url = "https://" + host;
   return GURL(url);
 }
 
 // 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

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

unnecessary extra horizontal whitespace after the -1

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

Done.

 // 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
 // unless overridden by a command line flag.
 int64 GetExpirationDelta() {
   // Check command line flags first to give them priority, then check
   // experimental groups.
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kRememberCertErrorDecisions)) {
     std::string switch_value =
@@ skipping 33 matching lines @@
 }
 
 std::string GetKey(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());
   std::string base64_fingerprint;
   base::Base64Encode(
       base::StringPiece(reinterpret_cast<const char*>(fingerprint.data),

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

Is this reinterpret_cast<> necessary? Shouldn't the char[32] implicitly cast
into a const char*?

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

I was surprised, too, but Clang complains about "no matching constructor" for
StringPiece if I don't put in the reinterpret_cast<>.

                         sizeof(fingerprint.data)),
       &base64_fingerprint);
   return base::UintToString(error) + base64_fingerprint;
 }
 
 }  // namespace
 
 // This helper function gets the dictionary of certificate fingerprints to
 // errors of certificates that have been accepted by the user from the content
 // 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|,
 // 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

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

grammar is off here...perhaps s/be/being/?

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

Done.

 // |CreateDictionaryEntries|, if no dictionary is found or the decisions are
 // expired, a new dictionary will be created

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

Missing '.' at the end.

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

Done.

 base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict(

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

You should directly include "base/values.h" for this rather than rely on
transitive includes.

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

Done.

     base::DictionaryValue* dict,

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

Not for readability, and don't worry, the length of this comment is not
correlated to its importance. This is just for you to think about and evaluate
on your own, and readability review is a good time for someone to present these
ideas to you. And since readability reviewers have the perhaps unjustified
perception of being reasonably experienced/knowledgeable here, a short rant will
hopefully be less presumptuous/overbearing than some rando.

My personal feeling here is that |dict| is confusingly used for both input and
output. It's not immediately obvious if the output will be the return value or
in |dict|. And the ownership of |cert_error_dict| is a little weird. The
implicit function contract seems to be "I stored it nested somewhere within
|dict|, and here's a raw pointer to it". But this contract isn't explicit, which
means that there's tight coupling between the caller and the callee, since the
caller needs to understand what the callee is doing here.

Generally, separate functions should try to remain decoupled, since one of the
benefits of functions (besides code reuse), is having a smaller unit of
comprehension. One example of how you could get this is by returning a
scoped_ptr<base::DictionaryValue> instead, and having the caller set the
DictionaryValue in the right place. And if you really wanted the code reuse, you
could wrap those few lines of conditional checking in another helper function.

An analogy for this is tech support for my dad. Let's say I'm installing Skype
for him. He gives me his computer, I install Skype, and put a shortcut to the
Skype executable on the desktop. Ignoring the fact that desktop shortcuts
probably tell where they point to, whereas Foo* just gives some address in
memory, now my dad doesn't know where Skype is. He'd have to know where I
generally put new applications. Instead, a cleaner interface would be for me to
install it to the desktop, and tell him it's his job to move it into the
appropriate location. This requires a bit more work on his part, but makes
everything more explicit.

In programming terms, I paid the cost of a little loss of code reuse (which I
could write another helper function to solve, and in this analogy, this helper
function would be moving the application to the default installation directory
and leaving a shortcut on the desktop) in order to eliminate a side effect and
have a more explicit input/output arrangement.

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

It sounds like we might want to change this but in a different (non-readability)
CL.

     CreateDictionaryEntriesDisposition create_entries,
     bool* expired_previous_decision) {
   // This needs to be done first in case the method is short circuited by an
   // early failure.
   *expired_previous_decision = false;
 
   // Extract the version of the certificate decision structure from the content
   // setting.
   int version;
   bool success = dict->GetInteger(kSSLCertDecisionVersionKey, &version);
@@ skipping 274 matching lines @@
   cert_dict->SetIntegerWithoutPathExpansion(GetKey(cert, error), judgment);
 
   // The map takes ownership of the value, so it is released in the call to
   // SetWebsiteSetting.
   map->SetWebsiteSetting(pattern,
                          pattern,
                          CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS,
                          std::string(),
                          value.release());
 }
+