Disk Based Certificate Cache Implementation

net/http/disk_based_cert_cache.h

+// Copyright (c) 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.
+
+#ifndef NET_HTTP_DISK_BASED_CERT_CACHE_H
+#define NET_HTTP_DISK_BASED_CERT_CACHE_H
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_export.h"
+#include "net/cert/x509_certificate.h"
+#include "net/disk_cache/disk_cache.h"

[Ryan Sleevi, 2014/06/17 00:00:21]

As per http://www.chromium.org/developers/coding-style/cpp-dos-and-donts , you
can forward declare disk_cache::Backend , and avoid including this header

namespace net {

namespace disk_cache {
class Backend;
}  // namespace disk_cache

class ... {
};

}  // namespace net

+
+namespace net {
+
+class NET_EXPORT_PRIVATE DiskBasedCertCache {
+ public:
+  typedef base::Callback<void(const std::string&)> SetCallback;
+  typedef base::Callback<void(X509Certificate::OSCertHandle cert_handle)>

[Ryan Sleevi, 2014/06/17 00:00:21]

STYLE: In this case, you likely want to ensure your Get/Set pairs are
consistently ordered.

Your types use Set/Get, your methods use Get/Set, your members use Write/Read,
and helper methods use Write/Read

+      GetCallback;
+
+  // Constructor takes in a previously initialized backend, which is then
+  // used to store the certificates in the cache.
+  explicit DiskBasedCertCache(disk_cache::Backend* backend);
+  ~DiskBasedCertCache();
+
+  // Fetches the certificate associated with |key|. If the certificate is
+  // found within the cache, |cb| will be called with the certificate.
+  // Otherwise, |cb| will be called with NULL.

[Ryan Sleevi, 2014/06/17 00:00:21]

Will |cb| ever be called synchronously? Good to document either way.

Also, document ownership of the cert_handle (namely, that it's not transferred)

+  void Get(std::string& key, GetCallback cb);

[Ryan Sleevi, 2014/06/17 00:00:21]

STYLE: Reference arguments should always be passed as const - "std::string& key"
-> "const std::string& key"

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

STYLE: Callbacks should always be passed as const-references (we generally never
pass values by-value, with the exception of scoped_ptr<>

+
+  // Stores |cert_handle| in the cache. If |cert_handle| is successfully stored,
+  // |cb| will be called with the key. If |cb| is called with an empty
+  // std::string, then |handle| was not stored.

[Ryan Sleevi, 2014/06/17 00:00:21]

empty std::string -> empty string

+  void Set(const X509Certificate::OSCertHandle cert_handle, SetCallback cb);

[Ryan Sleevi, 2014/06/17 00:00:21]

And with the exception of above, you need to pass the OSCertHandle by value ;)

+
+  base::WeakPtr<DiskBasedCertCache> GetWeakPtr() {

[Ryan Sleevi, 2014/06/17 00:00:21]

DESIGN: Exposing a public member to allow access to a WeakPtr is generally a
"code smell". No one should ever ask you for your WeakPtr (nor create one of
you) - you should give it to them (via a callback)

+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  // Types --------------------------------------------------------------------

[Ryan Sleevi, 2014/06/17 00:00:21]

STYLE: Don't include these block-style comments ( see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic...
)

+  class WriteWorker;
+  class ReadWorker;
+
+  typedef base::hash_map<std::string, WriteWorker*> WriteWorkerMap;
+  typedef base::hash_map<std::string, ReadWorker*> ReadWorkerMap;

[Ryan Sleevi, 2014/06/17 00:00:21]

You use base::hash_map, but you never include the headers that define it.

You need to do

#include "base/containers/hash_tables.h"

+
+  // Methods ------------------------------------------------------------------
+  std::string Key(const X509Certificate::OSCertHandle cert_handle) const;

[Ryan Sleevi, 2014/06/17 00:00:21]

STYLE: Function names are generally of the form of VerbNoun if they're mutating,
and always field_name if they're accessors

In this case, a better name might be GetCacheKeyForCert()

However, this private method doesn't depend on any of the other data members
within this class - it's better suited as simply a helper function within an
unnamed namespace in the .cc, never even appearing on the class / in the .h

+  void FinishedWriteOperation(const std::string& key);
+  void FinishedReadOperation(const std::string& key);

[Ryan Sleevi, 2014/06/17 00:00:21]

Document (Briefly) what these methods do.

+
+  // Variables ----------------------------------------------------------------
+  WriteWorkerMap write_worker_map_;
+  ReadWorkerMap read_worker_map_;
+
+  disk_cache::Backend* backend_;
+  base::WeakPtrFactory<DiskBasedCertCache> weak_factory_;
+
+  base::Callback<void(const std::string&)> write_io_callback_;
+  base::Callback<void(const std::string&)> read_io_callback_;
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_DISK_BASED_CERT_CACHE_H