DiskBasedCertCache method name change + readability fixes.

net/http/disk_based_cert_cache.cc

 // 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.
 
 #include "net/http/disk_based_cert_cache.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 21 matching lines @@
 
   return "cert:" +
          base::HexEncode(fingerprint.data, arraysize(fingerprint.data));
 }
 
 enum CacheResult {
   MEMORY_CACHE_HIT = 0,
   DISK_CACHE_HIT,
   DISK_CACHE_ENTRY_CORRUPT,
   DISK_CACHE_ERROR,
-  CACHE_MISS,
   CACHE_RESULT_MAX
 };
 
 void RecordCacheResult(CacheResult result) {
   UMA_HISTOGRAM_ENUMERATION(
       "DiskBasedCertCache.CertIoCacheResult", result, CACHE_RESULT_MAX);
 }
 
 }  // namespace
 
-// WriteWorkers represent pending Set jobs in the DiskBasedCertCache. Each
-// certificate requested to be cached is assigned a Writeworker on a one-to-one
-// basis. The same certificate should not have multiple WriteWorkers at the same
+// WriteWorkers represent pending SetCertificate jobs in the DiskBasedCertCache.
+// Each certificate requested to be stored is assigned a WriteWorker.
+// The same certificate should not have multiple WriteWorkers at the same
 // time; instead, add a user callback to the existing WriteWorker.
 class DiskBasedCertCache::WriteWorker {
  public:
   // |backend| is the backend to store |certificate| in, using
   // |key| as the key for the disk_cache::Entry.
   // |cleanup_callback| is called to clean up this ReadWorker,
   // regardless of success or failure.
   WriteWorker(disk_cache::Backend* backend,
               const std::string& key,
               X509Certificate::OSCertHandle cert_handle,
@@ skipping 39 matching lines @@
 
   // Invokes all of the |user_callbacks_|
   void RunCallbacks(int rv);
 
   disk_cache::Backend* backend_;
   const X509Certificate::OSCertHandle cert_handle_;
   std::string key_;
   bool canceled_;
 
   disk_cache::Entry* entry_;
-  State state_;
+  State next_state_;
   scoped_refptr<IOBuffer> buffer_;
   int io_buf_len_;
 
   base::Closure cleanup_callback_;
   std::vector<SetCallback> user_callbacks_;
   CompletionCallback io_callback_;
 };
 
 DiskBasedCertCache::WriteWorker::WriteWorker(
     disk_cache::Backend* backend,
     const std::string& key,
     X509Certificate::OSCertHandle cert_handle,
     const base::Closure& cleanup_callback)
     : backend_(backend),
       cert_handle_(X509Certificate::DupOSCertHandle(cert_handle)),
       key_(key),
       canceled_(false),
       entry_(NULL),
-      state_(STATE_NONE),
+      next_state_(STATE_NONE),
       io_buf_len_(0),
       cleanup_callback_(cleanup_callback),
       io_callback_(
           base::Bind(&WriteWorker::OnIOComplete, base::Unretained(this))) {
 }
 
 DiskBasedCertCache::WriteWorker::~WriteWorker() {
   if (cert_handle_)
     X509Certificate::FreeOSCertHandle(cert_handle_);
   if (entry_)
     entry_->Close();
 }
 
 void DiskBasedCertCache::WriteWorker::Start() {
-  DCHECK_EQ(STATE_NONE, state_);
-  state_ = STATE_CREATE;
+  DCHECK_EQ(STATE_NONE, next_state_);
+  next_state_ = STATE_CREATE;
   int rv = DoLoop(OK);
 
   if (rv == ERR_IO_PENDING)
     return;
 
   Finish(rv);
 }
 
 void DiskBasedCertCache::WriteWorker::AddCallback(
     const SetCallback& user_callback) {
@@ skipping 13 matching lines @@
   rv = DoLoop(rv);
 
   if (rv == ERR_IO_PENDING)
     return;
 
   Finish(rv);
 }
 
 int DiskBasedCertCache::WriteWorker::DoLoop(int rv) {
   do {
-    State next_state = state_;
-    state_ = STATE_NONE;
-    switch (next_state) {
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
       case STATE_CREATE:
         rv = DoCreate();
         break;
       case STATE_CREATE_COMPLETE:
         rv = DoCreateComplete(rv);
         break;
       case STATE_OPEN:
         rv = DoOpen();
         break;
       case STATE_OPEN_COMPLETE:
         rv = DoOpenComplete(rv);
         break;
       case STATE_WRITE:
         rv = DoWrite();
         break;
       case STATE_WRITE_COMPLETE:
         rv = DoWriteComplete(rv);
         break;
       case STATE_NONE:
         NOTREACHED();
         break;
     }
-  } while (rv != ERR_IO_PENDING && state_ != STATE_NONE);
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   return rv;
 }
 
 int DiskBasedCertCache::WriteWorker::DoCreate() {
-  state_ = STATE_CREATE_COMPLETE;
+  next_state_ = STATE_CREATE_COMPLETE;
 
   return backend_->CreateEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::WriteWorker::DoCreateComplete(int rv) {
   // An error here usually signifies that the entry already exists.
   // If this occurs, it is necessary to instead open the previously
   // existing entry.
   if (rv < 0) {
-    state_ = STATE_OPEN;
+    next_state_ = STATE_OPEN;
     return OK;
   }
 
-  state_ = STATE_WRITE;
+  next_state_ = STATE_WRITE;
   return OK;
 }
 
 int DiskBasedCertCache::WriteWorker::DoOpen() {
-  state_ = STATE_OPEN_COMPLETE;
+  next_state_ = STATE_OPEN_COMPLETE;
   return backend_->OpenEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::WriteWorker::DoOpenComplete(int rv) {
   if (rv < 0)
     return rv;
 
-  state_ = STATE_WRITE;
+  next_state_ = STATE_WRITE;
   return OK;
 }
 
 int DiskBasedCertCache::WriteWorker::DoWrite() {
   std::string write_data;
   bool encoded = X509Certificate::GetDEREncoded(cert_handle_, &write_data);
 
   if (!encoded)
     return ERR_FAILED;
 
   buffer_ = new IOBuffer(write_data.size());
   io_buf_len_ = write_data.size();
   memcpy(buffer_->data(), write_data.data(), io_buf_len_);
 
-  state_ = STATE_WRITE_COMPLETE;
+  next_state_ = STATE_WRITE_COMPLETE;
 
   return entry_->WriteData(0 /* index */,
                            0 /* offset */,
                            buffer_,
                            write_data.size(),
                            io_callback_,
                            true /* truncate */);
 }
 
 int DiskBasedCertCache::WriteWorker::DoWriteComplete(int rv) {
@@ skipping 16 matching lines @@
     key = key_;
 
   for (std::vector<SetCallback>::const_iterator it = user_callbacks_.begin();
        it != user_callbacks_.end();
        ++it) {
     it->Run(key);
   }
   user_callbacks_.clear();
 }
 
-// ReadWorkers represent pending Get jobs in the DiskBasedCertCache. Each
-// certificate requested to be retrieved from the cache is assigned a ReadWorker
-// on a one-to-one basis. The same |key| should not have multiple ReadWorkers
-// at the same time; instead, call AddCallback to add a user_callback_ to
-// the the existing ReadWorker.
+// ReadWorkers represent pending GetCertificate jobs in the DiskBasedCertCache.
+// Each certificate requested to be retrieved from the cache is assigned a
+// ReadWorker. The same |key| should not have multiple ReadWorkers at the
+// same time; instead, call AddCallback to add a user callback to the
+// existing ReadWorker.
 class DiskBasedCertCache::ReadWorker {
  public:
   // |backend| is the backend to read |certificate| from, using
   // |key| as the key for the disk_cache::Entry.
   // |cleanup_callback| is called to clean up this ReadWorker,
   // regardless of success or failure.
   ReadWorker(disk_cache::Backend* backend,
              const std::string& key,
              const GetCallback& cleanup_callback);
 
@@ skipping 32 matching lines @@
   // Invokes all of |user_callbacks_|
   void RunCallbacks();
 
   disk_cache::Backend* backend_;
   X509Certificate::OSCertHandle cert_handle_;
   std::string key_;
   bool canceled_;
 
   disk_cache::Entry* entry_;
 
-  State state_;
+  State next_state_;
   scoped_refptr<IOBuffer> buffer_;
   int io_buf_len_;
 
   GetCallback cleanup_callback_;
   std::vector<GetCallback> user_callbacks_;
   CompletionCallback io_callback_;
 };
 
 DiskBasedCertCache::ReadWorker::ReadWorker(disk_cache::Backend* backend,
                                            const std::string& key,
                                            const GetCallback& cleanup_callback)
     : backend_(backend),
       cert_handle_(NULL),
       key_(key),
       canceled_(false),
       entry_(NULL),
-      state_(STATE_NONE),
+      next_state_(STATE_NONE),
       io_buf_len_(0),
       cleanup_callback_(cleanup_callback),
       io_callback_(
           base::Bind(&ReadWorker::OnIOComplete, base::Unretained(this))) {
 }
 
 DiskBasedCertCache::ReadWorker::~ReadWorker() {
   if (entry_)
     entry_->Close();
   if (cert_handle_)
     X509Certificate::FreeOSCertHandle(cert_handle_);
 }
 
 void DiskBasedCertCache::ReadWorker::Start() {
-  DCHECK_EQ(STATE_NONE, state_);
-  state_ = STATE_OPEN;
+  DCHECK_EQ(STATE_NONE, next_state_);
+  next_state_ = STATE_OPEN;
   int rv = DoLoop(OK);
 
   if (rv == ERR_IO_PENDING)
     return;
 
   Finish(rv);
 }
 
 void DiskBasedCertCache::ReadWorker::AddCallback(
     const GetCallback& user_callback) {
@@ skipping 13 matching lines @@
   rv = DoLoop(rv);
 
   if (rv == ERR_IO_PENDING)
     return;
 
   Finish(rv);
 }
 
 int DiskBasedCertCache::ReadWorker::DoLoop(int rv) {
   do {
-    State next_state = state_;
-    state_ = STATE_NONE;
-    switch (next_state) {
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
       case STATE_OPEN:
         rv = DoOpen();
         break;
       case STATE_OPEN_COMPLETE:
         rv = DoOpenComplete(rv);
         break;
       case STATE_READ:
         rv = DoRead();
         break;
       case STATE_READ_COMPLETE:
         rv = DoReadComplete(rv);
         break;
       case STATE_NONE:
         NOTREACHED();
         break;
     }
-  } while (rv != ERR_IO_PENDING && state_ != STATE_NONE);
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   return rv;
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpen() {
-  state_ = STATE_OPEN_COMPLETE;
+  next_state_ = STATE_OPEN_COMPLETE;
   return backend_->OpenEntry(key_, &entry_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoOpenComplete(int rv) {
   if (rv < 0) {

[wtc, 2014/08/04 22:44:42]

It seems useful to be able to distinguish between a cache miss and a cache
error. Can we change OpenEntry to return ERR_CACHE_MISS or ERR_FILE_NOT_FOUND in
the cache miss case?

[rvargas (doing something else), 2014/08/04 22:59:48]

What would you do with specific codes other than log the result and move on?

Errors should be logged at the lower level (close to where they actually
happen), and the code close to the interface basically just returns the entry if
found, so failure is pretty much not found (but ERR_CACHE_MISS has a very
specific meaning, returned by the HTTP cache, and FILE_NOT_FOUND suggests a
specific OS level error to me). That is to say, I favor code that doesn't rely
on specific error codes unless really needed.

-    // Errors other than ERR_CACHE_MISS are not recorded as either a hit
-    // or a miss.
-    RecordCacheResult(rv == ERR_CACHE_MISS ? CACHE_MISS : DISK_CACHE_ERROR);
+    RecordCacheResult(DISK_CACHE_ERROR);
     return rv;
   }
 
-  state_ = STATE_READ;
+  next_state_ = STATE_READ;
   return OK;
 }
 
 int DiskBasedCertCache::ReadWorker::DoRead() {
-  state_ = STATE_READ_COMPLETE;
+  next_state_ = STATE_READ_COMPLETE;
   io_buf_len_ = entry_->GetDataSize(0 /* index */);
   buffer_ = new IOBuffer(io_buf_len_);
   return entry_->ReadData(
       0 /* index */, 0 /* offset */, buffer_, io_buf_len_, io_callback_);
 }
 
 int DiskBasedCertCache::ReadWorker::DoReadComplete(int rv) {
   // The cache should return the entire buffer length. If it does not,
   // it is probably indicative of an issue other than corruption.
   if (rv < io_buf_len_) {
@@ skipping 47 matching lines @@
        ++it) {
     it->second->Cancel();
   }
   for (ReadWorkerMap::iterator it = read_worker_map_.begin();
        it != read_worker_map_.end();
        ++it) {
     it->second->Cancel();
   }
 }
 
-void DiskBasedCertCache::Get(const std::string& key, const GetCallback& cb) {
+void DiskBasedCertCache::GetCertificate(const std::string& key,
+                                        const GetCallback& cb) {
   DCHECK(!key.empty());
 
   // If the handle is already in the MRU cache, just return that (via callback).
   // Note, this will also bring the cert_handle to the front of the recency
   // list in the MRU cache.
   MRUCertCache::iterator mru_it = mru_cert_cache_.Get(key);
   if (mru_it != mru_cert_cache_.end()) {
     RecordCacheResult(MEMORY_CACHE_HIT);
     ++mem_cache_hits_;
     cb.Run(mru_it->second);
@@ skipping 11 matching lines @@
                                   weak_factory_.GetWeakPtr(),
                                   key));
     read_worker_map_[key] = worker;
     worker->AddCallback(cb);
     worker->Start();
   } else {
     it->second->AddCallback(cb);
   }
 }
 
-void DiskBasedCertCache::Set(const X509Certificate::OSCertHandle cert_handle,
-                             const SetCallback& cb) {
+void DiskBasedCertCache::SetCertificate(
+    const X509Certificate::OSCertHandle cert_handle,
+    const SetCallback& cb) {
   DCHECK(!cb.is_null());
   DCHECK(cert_handle);
   std::string key = GetCacheKeyForCert(cert_handle);
 
   WriteWorkerMap::iterator it = write_worker_map_.find(key);
 
   if (it == write_worker_map_.end()) {
     WriteWorker* worker =
         new WriteWorker(backend_,
                         key,
@@ skipping 20 matching lines @@
 
 void DiskBasedCertCache::FinishedWriteOperation(
     const std::string& key,
     X509Certificate::OSCertHandle cert_handle) {
   write_worker_map_.erase(key);
   if (!key.empty())
     mru_cert_cache_.Put(key, X509Certificate::DupOSCertHandle(cert_handle));
 }
 
 }  // namespace net