Added net_log logging statments for CertVerifyResult

net/cert/multi_threaded_cert_verifier.cc

 // Copyright (c) 2012 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 "net/cert/multi_threaded_cert_verifier.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #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"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_certificate_net_log_param.h"
 
 #if defined(USE_NSS) || defined(OS_IOS)
 #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 {
 
 ////////////////////////////////////////////////////////////////////////////
 
 // Life of a request:
 //
 // MultiThreadedCertVerifier  CertVerifierJob  CertVerifierWorker     Request
 //      |                                         (origin loop)    (worker loop)
 //      |
 //   Verify()
@@ skipping 304 matching lines @@
         NetLog::TYPE_CERT_VERIFIER_REQUEST_BOUND_TO_JOB,
         net_log_.source().ToEventParametersCallback());
 
     requests_.push_back(request);
   }
 
   void HandleResult(
       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,
                                base::TimeDelta::FromMilliseconds(1),
                                base::TimeDelta::FromMinutes(10),
                                100);
     if (is_first_job) {
       UMA_HISTOGRAM_CUSTOM_TIMES("Net.CertVerifier_First_Job_Latency",
                                  latency,
                                  base::TimeDelta::FromMilliseconds(1),
@@ skipping 211 matching lines @@
 }
 
 void MultiThreadedCertVerifier::OnCACertChanged(
     const X509Certificate* cert) {
   DCHECK(CalledOnValidThread());
 
   ClearCache();
 }
 
 }  // namespace net
+