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 32 matching lines @@
   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) {
-    // 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);

[wtc, 2014/08/01 03:03:55]

IMPORTANT: can you explain this change? With this change, the CACHE_MISS cache
result becomes unused.

[brandonsalmon, 2014/08/01 22:14:29]

Yep, that was intentional.

When I was originally designing this I wanted to differentiate between cache
misses and other errors, so I looked up the error code for cache misses.
Unfortunately, I made a mistake by not checking if OpenEntry actually returns
ERR_CACHE_MISS. It turns out it doesn't, if I remember correctly it just returns
ERR_FAILED.

As a result of rv not being ERR_CACHE_MISS, cache misses have been recorded as
DISK_CACHE_ERROR. Here I am modifying the code to reflect that.

     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