Added net_log logging statments for CertVerifyResult

net/cert/multi_threaded_cert_verifier.cc

Index: net/cert/multi_threaded_cert_verifier.cc
diff --git a/net/cert/multi_threaded_cert_verifier.cc b/net/cert/multi_threaded_cert_verifier.cc
index f1b9bb8d1faf19c95a9b231314bebeb6528bc3fd..4ccef43e33f1c9ca5632513226bd12eeb172583c 100644
--- a/net/cert/multi_threaded_cert_verifier.cc
+++ b/net/cert/multi_threaded_cert_verifier.cc
@@ -15,8 +15,11 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
+#include "base/values.h"
+#include "net/base/hash_value.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
+#include "net/cert/cert_status_flags.h"

[Ryan Sleevi, 2014/05/31 02:41:11]

I don't believe you need this header anymore? We're just using the numeric value
from verify_result.cert_status for now

[mshelley, 2014/06/02 20:40:06]

Done.

 #include "net/cert/cert_trust_anchor_provider.h"
 #include "net/cert/cert_verify_proc.h"
 #include "net/cert/crl_set.h"
@@ -27,6 +30,38 @@
 #include <private/pprthred.h>  // PR_DetachThread
 #endif
 
+namespace {

[wtc, 2014/05/31 02:48:03]

I suggest that you nest this unnamed namespace inside the net namespace, so that
the CertVerifyResultCallback function can omit the net:: prefix.

namespace net {

namespace {

base::Value* CertVerifyResultCallback(const CertVerifyResult& verify_result,
...
}

}  // namespace

...

}  // namespace net

[mshelley, 2014/06/02 20:40:06]

Done.

+
+base::Value* CertVerifyResultCallback(net::CertVerifyResult verify_result,

[Ryan Sleevi, 2014/05/31 02:41:11]

STYLE: So, in the previous comment, I mentioned you could move this entire
function to line 115

Reasoning:

1) We generally prefer one unnamed namespace in a file, that contains all of the
'helper' functions
2) The choice of 115 (versus, say, 109 or 112) is to match the recommended order
of declarations
3) It allows you to delete the "net::" on lines 35, 36, and 49

[wtc, 2014/05/31 02:48:03]

Since CertVerifyResult is a struct with several members, I think this parameter
should be declared as a const reference:

    const net::CertVerifyResult& verify_result

See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

[mshelley, 2014/06/02 20:40:06]

Done.

+                                      net::NetLog::LogLevel log_level) {
+  base::DictionaryValue* results = new base::DictionaryValue();
+  results->SetBoolean("has_md5", verify_result.has_md5);
+  results->SetBoolean("has_md2", verify_result.has_md2);
+  results->SetBoolean("has_md4", verify_result.has_md4);
+  results->SetBoolean("is_issued_by_known_root",
+                      verify_result.is_issued_by_known_root);
+  results->SetBoolean("is_issued_by_additional_trust_anchor",
+                      verify_result.is_issued_by_additional_trust_anchor);
+  results->SetBoolean("common_name_fallback_used",
+                      verify_result.common_name_fallback_used);
+  results->SetInteger("cert_status", verify_result.cert_status);
+  results->Set("verified_cert",
+               net::NetLogX509CertificateCallback(verify_result.verified_cert,
+                                                  log_level));
+  base::ListValue* hashes = new base::ListValue();
+  for (std::vector<net::HashValue>::const_iterator it =
+           verify_result.public_key_hashes.begin();
+       it != verify_result.public_key_hashes.end();
+       it++) {

[wtc, 2014/05/31 02:48:03]

Use pre-increment to increment an iterator. See the Style Guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...

[mshelley, 2014/06/02 20:40:06]

Done.

+    hashes->AppendString(it->ToString());
+  }
+
+  results->Set("public_key_hashes", hashes);
+
+  return results;
+}
+}

[wtc, 2014/05/31 02:48:03]

1. Add a blank line before this line.

2. This line is the closing '}' of a namespace. Our Style Guide recommends that
we add a comment:

}  // namespace

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...

[mshelley, 2014/06/02 20:40:06]

Done.

+
 namespace net {
 
 ////////////////////////////////////////////////////////////////////////////
@@ -351,7 +386,9 @@ class CertVerifierJob {
       const MultiThreadedCertVerifier::CachedResult& verify_result,
       bool is_first_job) {
     worker_ = NULL;
-    net_log_.EndEvent(NetLog::TYPE_CERT_VERIFIER_JOB);
+    net_log_.EndEvent(
+        NetLog::TYPE_CERT_VERIFIER_JOB,
+        base::Bind(&CertVerifyResultCallback, verify_result.result));
     base::TimeDelta latency = base::TimeTicks::Now() - start_time_;
     UMA_HISTOGRAM_CUSTOM_TIMES("Net.CertVerifier_Job_Latency",
                                latency,
@@ -583,3 +620,4 @@ void MultiThreadedCertVerifier::OnCACertChanged(
 }
 
 }  // namespace net
+