Clean up PnaclTranslationCache interface

chrome/browser/nacl_host/pnacl_translation_cache.cc

Index: chrome/browser/nacl_host/pnacl_translation_cache.cc
diff --git a/chrome/browser/nacl_host/pnacl_translation_cache.cc b/chrome/browser/nacl_host/pnacl_translation_cache.cc
index 6492e36d7411ed807c6039167151f2847ea8329b..d207e8464aa2984c32a156a0b847fb93605de2d1 100644
--- a/chrome/browser/nacl_host/pnacl_translation_cache.cc
+++ b/chrome/browser/nacl_host/pnacl_translation_cache.cc
@@ -8,12 +8,15 @@
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_checker.h"
+#include "components/nacl/common/pnacl_types.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/disk_cache.h"
 
+using base::IntToString;
 using content::BrowserThread;
 
 namespace {
@@ -23,7 +26,7 @@ void CloseDiskCacheEntry(disk_cache::Entry* entry) { entry->Close(); }
 }  // namespace
 
 namespace pnacl {
-// These are in pnacl_cache namespace instead of static so they can be used
+// These are in pnacl namespace instead of static so they can be used
 // by the unit test.
 const int kMaxDiskCacheSize = 1000 * 1024 * 1024;
 const int kMaxMemCacheSize = 100 * 1024 * 1024;
@@ -38,12 +41,16 @@ const int kMaxMemCacheSize = 100 * 1024 * 1024;
 class PnaclTranslationCacheEntry
     : public base::RefCounted<PnaclTranslationCacheEntry> {
  public:
-  PnaclTranslationCacheEntry(base::WeakPtr<PnaclTranslationCache> cache,
-                             const std::string& key,
-                             std::string* read_nexe,
-                             const std::string& write_nexe,
-                             const CompletionCallback& callback,
-                             bool is_read);
+  static PnaclTranslationCacheEntry* GetReadEntry(
+      base::WeakPtr<PnaclTranslationCache> cache,
+      const std::string& key,
+      const NexeCallback& callback);
+  static PnaclTranslationCacheEntry* GetWriteEntry(
+      base::WeakPtr<PnaclTranslationCache> cache,
+      const std::string& key,
+      net::DrainableIOBuffer* write_nexe,
+      const CompletionCallback& callback);
+
   void Start();
 
   // Writes:                                ---
@@ -66,6 +73,9 @@ class PnaclTranslationCacheEntry
 
  private:
   friend class base::RefCounted<PnaclTranslationCacheEntry>;
+  PnaclTranslationCacheEntry(base::WeakPtr<PnaclTranslationCache> cache,
+                             const std::string& key,
+                             bool is_read);
   ~PnaclTranslationCacheEntry();
 
   // Try to open an existing entry in the backend
@@ -84,47 +94,57 @@ class PnaclTranslationCacheEntry
   // Used as the callback for all operations to the backend. Handle state
   // transitions, track bytes transferred, and call the other helper methods.
   void DispatchNext(int rv);
-  // Get the total transfer size. For reads, must be called after the backend
-  // entry has been opened.
-  int GetTransferSize();
 
   base::WeakPtr<PnaclTranslationCache> cache_;
-
   std::string key_;
-  std::string* read_nexe_;
-  std::string write_nexe_;
   disk_cache::Entry* entry_;
   CacheStep step_;
   bool is_read_;
-  int bytes_transferred_;
-  int bytes_to_transfer_;
-  scoped_refptr<net::IOBufferWithSize> read_buf_;
-  CompletionCallback finish_callback_;
+  NexeCallback read_callback_;
+  CompletionCallback write_callback_;
+  scoped_refptr<net::DrainableIOBuffer> io_buf_;
   base::ThreadChecker thread_checker_;
   DISALLOW_COPY_AND_ASSIGN(PnaclTranslationCacheEntry);
 };
 
+// static
+PnaclTranslationCacheEntry* PnaclTranslationCacheEntry::GetReadEntry(
+    base::WeakPtr<PnaclTranslationCache> cache,
+    const std::string& key,
+    const NexeCallback& callback) {
+  PnaclTranslationCacheEntry* entry(
+      new PnaclTranslationCacheEntry(cache, key, true));
+  entry->read_callback_ = callback;
+  return entry;
+}
+
+// static
+PnaclTranslationCacheEntry* PnaclTranslationCacheEntry::GetWriteEntry(
+    base::WeakPtr<PnaclTranslationCache> cache,
+    const std::string& key,
+    net::DrainableIOBuffer* write_nexe,
+    const CompletionCallback& callback) {
+  PnaclTranslationCacheEntry* entry(
+      new PnaclTranslationCacheEntry(cache, key, false));
+  entry->io_buf_ = write_nexe;
+  entry->write_callback_ = callback;
+  return entry;
+}
+
 PnaclTranslationCacheEntry::PnaclTranslationCacheEntry(
     base::WeakPtr<PnaclTranslationCache> cache,
     const std::string& key,
-    std::string* read_nexe,
-    const std::string& write_nexe,
-    const CompletionCallback& callback,
     bool is_read)
     : cache_(cache),
       key_(key),
-      read_nexe_(read_nexe),
-      write_nexe_(write_nexe),
       entry_(NULL),
       step_(UNINITIALIZED),
-      is_read_(is_read),
-      bytes_transferred_(0),
-      bytes_to_transfer_(-1),
-      finish_callback_(callback) {}
+      is_read_(is_read) {}
 
 PnaclTranslationCacheEntry::~PnaclTranslationCacheEntry() {
   // Ensure we have called the user's callback
-  DCHECK(finish_callback_.is_null());
+  DCHECK(read_callback_.is_null());
+  DCHECK(write_callback_.is_null());
 }
 
 void PnaclTranslationCacheEntry::Start() {
@@ -154,12 +174,11 @@ void PnaclTranslationCacheEntry::CreateEntry() {
 }
 
 void PnaclTranslationCacheEntry::WriteEntry(int offset, int len) {
-  scoped_refptr<net::StringIOBuffer> io_buf =
-      new net::StringIOBuffer(write_nexe_.substr(offset, len));
+  DCHECK(io_buf_->BytesRemaining() == len);
   int rv = entry_->WriteData(
       1,
       offset,
-      io_buf.get(),
+      io_buf_.get(),
       len,
       base::Bind(&PnaclTranslationCacheEntry::DispatchNext, this),
       false);
@@ -168,25 +187,16 @@ void PnaclTranslationCacheEntry::WriteEntry(int offset, int len) {
 }
 
 void PnaclTranslationCacheEntry::ReadEntry(int offset, int len) {
-  read_buf_ = new net::IOBufferWithSize(len);
   int rv = entry_->ReadData(
       1,
       offset,
-      read_buf_.get(),
+      io_buf_.get(),
       len,
       base::Bind(&PnaclTranslationCacheEntry::DispatchNext, this));
   if (rv != net::ERR_IO_PENDING)
     DispatchNext(rv);
 }
 
-int PnaclTranslationCacheEntry::GetTransferSize() {
-  if (is_read_) {
-    DCHECK(entry_);
-    return entry_->GetDataSize(1);
-  }
-  return write_nexe_.size();
-}
-
 void PnaclTranslationCacheEntry::CloseEntry(int rv) {
   DCHECK(entry_);
   if (rv < 0)
@@ -197,9 +207,16 @@ void PnaclTranslationCacheEntry::CloseEntry(int rv) {
 }
 
 void PnaclTranslationCacheEntry::Finish(int rv) {
-  if (!finish_callback_.is_null()) {
-    finish_callback_.Run(rv);
-    finish_callback_.Reset();
+  if (is_read_) {
+    if (!read_callback_.is_null()) {
+      read_callback_.Run(rv, io_buf_);
+      read_callback_.Reset();
+    }
+  } else {
+    if (!write_callback_.is_null()) {
+      write_callback_.Run(rv);
+      write_callback_.Reset();
+    }
   }
   cache_->OpComplete(this);
 }
@@ -217,26 +234,30 @@ void PnaclTranslationCacheEntry::DispatchNext(int rv) {
     case OPEN_ENTRY:
       if (rv == net::OK) {
         step_ = TRANSFER_ENTRY;
-        bytes_to_transfer_ = GetTransferSize();
-        is_read_ ? ReadEntry(0, bytes_to_transfer_)
-                 : WriteEntry(0, bytes_to_transfer_);
+        if (is_read_) {
+          int bytes_to_transfer = entry_->GetDataSize(1);
+          io_buf_ = new net::DrainableIOBuffer(
+              new net::IOBuffer(bytes_to_transfer), bytes_to_transfer);
+          ReadEntry(0, bytes_to_transfer);
+        } else {
+          WriteEntry(0, io_buf_->size());
+        }
       } else {
         if (is_read_) {
           // Just a cache miss, not necessarily an error.
           entry_ = NULL;
           Finish(rv);
-          break;
+        } else {
+          step_ = CREATE_ENTRY;
+          CreateEntry();
         }
-        step_ = CREATE_ENTRY;
-        CreateEntry();
       }
       break;
 
     case CREATE_ENTRY:
       if (rv == net::OK) {
         step_ = TRANSFER_ENTRY;
-        bytes_to_transfer_ = GetTransferSize();
-        WriteEntry(bytes_transferred_, bytes_to_transfer_ - bytes_transferred_);
+        WriteEntry(io_buf_->BytesConsumed(), io_buf_->BytesRemaining());
       } else {
         LOG(ERROR) << "Failed to Create a PNaCl Translation Cache Entry";
         Finish(rv);
@@ -254,19 +275,19 @@ void PnaclTranslationCacheEntry::DispatchNext(int rv) {
         CloseEntry(rv);
         break;
       } else if (rv > 0) {
-        // For reads, copy the data that was just returned
-        if (is_read_)
-          read_nexe_->append(read_buf_->data(), rv);
-        bytes_transferred_ += rv;
-        if (bytes_transferred_ < bytes_to_transfer_) {
-          int len = bytes_to_transfer_ - bytes_transferred_;
-          is_read_ ? ReadEntry(bytes_transferred_, len)
-                   : WriteEntry(bytes_transferred_, len);
+        io_buf_->DidConsume(rv);
+        if (io_buf_->BytesRemaining() > 0) {
+          is_read_
+              ? ReadEntry(io_buf_->BytesConsumed(), io_buf_->BytesRemaining())
+              : WriteEntry(io_buf_->BytesConsumed(), io_buf_->BytesRemaining());
           break;
         }
       }
       // rv == 0 or we fell through (i.e. we have transferred all the bytes)
       step_ = CLOSE_ENTRY;
+      DCHECK(io_buf_->BytesConsumed() == io_buf_->size());
+      if (is_read_)
+        io_buf_->SetOffset(0);
       CloseEntry(0);
       break;
 
@@ -334,24 +355,23 @@ void PnaclTranslationCache::OnCreateBackendComplete(int rv) {
 // High-level API
 
 void PnaclTranslationCache::StoreNexe(const std::string& key,
-                                      const std::string& nexe) {
-  StoreNexe(key, nexe, CompletionCallback());
+                                      net::DrainableIOBuffer* nexe_data) {
+  StoreNexe(key, nexe_data, CompletionCallback());

[jvoung (off chromium), 2013/07/30 22:51:39]

Does this version of StoreNexe need to remain?

[Derek Schuff, 2013/07/31 00:04:16]

This is the one my current version of the code actually uses, but one of my
current TODOs is to keep the translation pending until the callback returns. so
it's still here for now.

 }
 
 void PnaclTranslationCache::StoreNexe(const std::string& key,
-                                      const std::string& nexe,
+                                      net::DrainableIOBuffer* nexe_data,
                                       const CompletionCallback& callback) {
-  PnaclTranslationCacheEntry* entry = new PnaclTranslationCacheEntry(
-      AsWeakPtr(), key, NULL, nexe, callback, false);
+  PnaclTranslationCacheEntry* entry = PnaclTranslationCacheEntry::GetWriteEntry(
+      AsWeakPtr(), key, nexe_data, callback);
   open_entries_[entry] = entry;
   entry->Start();
 }
 
 void PnaclTranslationCache::GetNexe(const std::string& key,
-                                    std::string* nexe,
-                                    const CompletionCallback& callback) {
-  PnaclTranslationCacheEntry* entry = new PnaclTranslationCacheEntry(
-      AsWeakPtr(), key, nexe, std::string(), callback, true);
+                                    const NexeCallback& callback) {
+  PnaclTranslationCacheEntry* entry =
+      PnaclTranslationCacheEntry::GetReadEntry(AsWeakPtr(), key, callback);
   open_entries_[entry] = entry;
   entry->Start();
 }
@@ -364,9 +384,7 @@ int PnaclTranslationCache::InitCache(const base::FilePath& cache_directory,
   if (in_memory_) {
     rv = InitWithMemBackend(kMaxMemCacheSize, callback);
   } else {
-    rv = InitWithDiskBackend(cache_directory,
-                             kMaxDiskCacheSize,
-                             callback);
+    rv = InitWithDiskBackend(cache_directory, kMaxDiskCacheSize, callback);
   }
 
   return rv;
@@ -378,4 +396,30 @@ int PnaclTranslationCache::Size() {
   return disk_cache_->GetEntryCount();
 }
 
+// static
+std::string PnaclTranslationCache::GetKey(const nacl::PnaclCacheInfo& info) {
+  if (!info.pexe_url.is_valid() || info.abi_version < 0 || info.opt_level < 0)
+    return std::string();
+  // Filter the username, password, and ref components from the URL
+  GURL::Replacements replacements;
+  replacements.ClearUsername();
+  replacements.ClearPassword();
+  replacements.ClearRef();
+  GURL key_url(info.pexe_url.ReplaceComponents(replacements));
+  std::string retval(key_url.spec());
+  retval += ";" + IntToString(info.abi_version) + ";" +

[jvoung (off chromium), 2013/07/30 22:51:39]

Could label the fields, so that it's easier to tell which part is which?

Should the non/less-user-controlled parts like the abi version and opt level
come first?

[Derek Schuff, 2013/07/31 00:04:16]

Done.

+            IntToString(info.opt_level) + ";";
+  // You would think that there is already code to format base::Time values
+  // somewhere, but I haven't found it yet. In any case, doing it ourselves
+  // here means we can keep the format stable.

[jvoung (off chromium), 2013/07/30 22:51:39]

There's strftime, but then you'd have to convert the base::Time to "struct tm". 
Otherwise, yeah you'd think there was a base method...

[Derek Schuff, 2013/07/31 00:04:16]

Yeah in the end I decided it's just as well that we control it ourselves.

+  base::Time::Exploded exploded;
+  info.last_modified.UTCExplode(&exploded);
+  retval += IntToString(exploded.year) + ":" + IntToString(exploded.month) +
+            ":" + IntToString(exploded.day_of_month) + ":" +
+            IntToString(exploded.hour) + ":" + IntToString(exploded.minute) +
+            ":" + IntToString(exploded.second) + ":" +
+            IntToString(exploded.millisecond) + ";";

[jvoung (off chromium), 2013/07/30 22:51:39]

Could label the time UTC.

[Derek Schuff, 2013/07/31 00:04:16]

Done.

+  retval += info.etag;
+  return retval;
+}
 }  // namespace pnacl