NetLogEventParameter to Callback refactoring 1,

net/base/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/base/multi_threaded_cert_verifier.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 134 matching lines @@
         flags_(flags),
         crl_set_(crl_set),
         origin_loop_(MessageLoop::current()),
         cert_verifier_(cert_verifier),
         canceled_(false),
         error_(ERR_FAILED) {
   }
 
   // Returns the certificate being verified. May only be called /before/
   // Start() is called.
-  X509Certificate* certificate() const { return cert_; }
+  scoped_refptr<X509Certificate> certificate() const { return cert_; }

[eroman, 2012/06/11 23:42:25]

Is this change necessary? Seems fine and I'm not suggesting you change it, but
wondering if it was actually necessary for correctness.

My understanding is it wouldn't matter if the extra reference was held from the
perspective of NetLog callback.

[mmenke, 2012/06/12 00:42:19]

It's not...But if it someone was being bad and keeping around a pointer to a ref
counted object before adding the first reference to it, and I stored it in a
refcounted temporary, bad things would happen.  Making this explicit makes life
less scary with a smart IDE that shows the return type of functions on mouse
over.

That having been said, since I've removed the code to add a ref, per your
suggestion, I've reverted this, since it's no longer an issue.

 
   bool Start() {
     DCHECK_EQ(MessageLoop::current(), origin_loop_);
 
     return base::WorkerPool::PostTask(
         FROM_HERE, base::Bind(&CertVerifierWorker::Run, base::Unretained(this)),
         true /* task is slow */);
   }
 
   // Cancel is called from the origin loop when the MultiThreadedCertVerifier is
@@ skipping 90 matching lines @@
 
 // A CertVerifierJob is a one-to-one counterpart of a CertVerifierWorker. It
 // lives only on the CertVerifier's origin message loop.
 class CertVerifierJob {
  public:
   CertVerifierJob(CertVerifierWorker* worker,
                   const BoundNetLog& net_log)
       : start_time_(base::TimeTicks::Now()),
         worker_(worker),
         net_log_(net_log) {
-    scoped_refptr<NetLog::EventParameters> params(
-        new X509CertificateNetLogParam(worker_->certificate()));
-    net_log_.BeginEvent(NetLog::TYPE_CERT_VERIFIER_JOB, params);
+    net_log_.BeginEvent(NetLog::TYPE_CERT_VERIFIER_JOB,
+        base::Bind(&NetLogX509CertificateNetLogCallback,
+                   worker_->certificate()));
   }
 
   ~CertVerifierJob() {
     if (worker_) {
       net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
       net_log_.EndEvent(NetLog::TYPE_CERT_VERIFIER_JOB, NULL);
       worker_->Cancel();
       DeleteAllCanceled();
     }
   }
 
   void AddRequest(CertVerifierRequest* request) {
     request->net_log().AddEvent(
         NetLog::TYPE_CERT_VERIFIER_REQUEST_BOUND_TO_JOB,
-        make_scoped_refptr(new NetLogSourceParameter(
-          "source_dependency", net_log_.source())));
+        net_log_.source().ToEventParametersCallback());
 
     requests_.push_back(request);
   }
 
   void HandleResult(
       const MultiThreadedCertVerifier::CachedResult& verify_result) {
     worker_ = NULL;
     net_log_.EndEvent(NetLog::TYPE_CERT_VERIFIER_JOB, NULL);
     UMA_HISTOGRAM_CUSTOM_TIMES("Net.CertVerifier_Job_Latency",
                                base::TimeTicks::Now() - start_time_,
@@ skipping 153 matching lines @@
   DCHECK(CalledOnValidThread());
 
   ClearCache();
 }
 
 void MultiThreadedCertVerifier::SetCertVerifyProc(CertVerifyProc* verify_proc) {
   verify_proc_ = verify_proc;
 }
 
 }  // namespace net