Improving and adding an in-memory MRU cache to DiskBasedCertCache.

net/http/disk_based_cert_cache.h

Index: net/http/disk_based_cert_cache.h
diff --git a/net/http/disk_based_cert_cache.h b/net/http/disk_based_cert_cache.h
index 302ac5cf7fe7a8d7aa259a4d36aac1b7ced5abd2..a7c51001b2efd38b997d5798120d7048e10ceedf 100644
--- a/net/http/disk_based_cert_cache.h
+++ b/net/http/disk_based_cert_cache.h
@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
+#include "base/containers/mru_cache.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/net_export.h"
 #include "net/cert/x509_certificate.h"
@@ -46,10 +47,28 @@ class NET_EXPORT_PRIVATE DiskBasedCertCache {
   void Set(const X509Certificate::OSCertHandle cert_handle,
            const SetCallback& cb);
 
+  // These functions intended for testing purposes only.

[wtc, 2014/07/02 20:51:50]

Nit: please document what they return.

[Ryan Sleevi, 2014/07/02 21:49:58]

For testing, we prefer to use friend classes to a test fixture. See
https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members

For Chromium, we have a special macro (FRIEND_TEST_ALL_PREFIXES) to handle this.

If it does need to be public, it should be named with "ForTesting" in the code,
so that the presubmit code will yell if it's used outside of a _unittest file

mem_cache_hits_for_testing()
mem_cache_misses_for_testing()

[rvargas (doing something else), 2014/07/02 22:14:39]

nit: If it is important that the methods should be for testing only, that should
be part of the name (FooForTest() or BlaForTesting()). If it is ok to expose
general getters, remove the comment and use variable_name().

+  int MemCacheHits() { return mem_cache_hits_; }
+  int MemCacheMisses() { return mem_cache_misses_; }

[wtc, 2014/07/02 20:51:50]

1. These methods should be declared const.

2. These getter methods should be named like the data members they return, as
recommended by our Style Guide.

So they should look like:

  int mem_cache_hits() const { return mem_cache_hits_; }
  int mem_cache_misses() const { return mem_cache_misses; }

[Ryan Sleevi, 2014/07/02 21:49:58]

http://www.chromium.org/developers/coding-style#TOC-Types

"Use unsigned integer types (preferably size_t) for object and allocation sizes,
object counts, array and pointer offsets, vector indices, and so on. "

+
  private:
   class ReadWorker;
   class WriteWorker;
 
+  // A functor used to free an OSCertHandle. Used by the MRUCertCache.
+  class CertFree {
+   public:
+    void operator()(X509Certificate::OSCertHandle cert_handle) {
+      X509Certificate::FreeOSCertHandle(cert_handle);
+    }
+  };

[Ryan Sleevi, 2014/07/02 21:49:58]

You should be able to just declare this class, and leave the definition in the
.cc

"struct CertFreeHelper"

If that doesn't work
"
struct CertFree {
  void operator()(X509Certificate::OSCertHandle handle);
};
"

and leave the definition in the .cc.

Note also the preference for structs for unary functors.

+
+  // An in-memory cache that is used to prevent redundant reads and writes
+  // to and from the disk cache.
+  typedef base::MRUCacheBase<std::string,
+                             X509Certificate::OSCertHandle,
+                             CertFree> MRUCertCache;
+
   // ReadWorkerMap and WriteWorkerMap map cache keys to their
   // corresponding Workers.
   typedef base::hash_map<std::string, ReadWorker*> ReadWorkerMap;
@@ -58,13 +77,20 @@ class NET_EXPORT_PRIVATE DiskBasedCertCache {
   // FinishedReadOperation and FinishedWriteOperation are used by callbacks
   // given to the workers to signal the DiskBasedCertCache they have completed
   // their work.
-  void FinishedReadOperation(const std::string& key);
-  void FinishedWriteOperation(const std::string& key);
+  void FinishedReadOperation(const std::string& key,
+                             X509Certificate::OSCertHandle cert_handle);
+  void FinishedWriteOperation(const std::string& key,
+                              X509Certificate::OSCertHandle cert_handle);
+
+  disk_cache::Backend* backend_;
 
   ReadWorkerMap read_worker_map_;
   WriteWorkerMap write_worker_map_;
+  MRUCertCache mru_cert_cache_;

[wtc, 2014/07/02 20:51:50]

Ricardo: does the disk cache already have an internal in-memory cache?

[rvargas (doing something else), 2014/07/02 22:14:39]

no, it doesn't

+
+  int mem_cache_hits_;
+  int mem_cache_misses_;
 
-  disk_cache::Backend* backend_;
   base::WeakPtrFactory<DiskBasedCertCache> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(DiskBasedCertCache);
 };