Add Keys() function to ServiceWorkerCache.

content/browser/service_worker/service_worker_cache.cc

Index: content/browser/service_worker/service_worker_cache.cc
diff --git a/content/browser/service_worker/service_worker_cache.cc b/content/browser/service_worker/service_worker_cache.cc
index 6a63b6b3e37d2dac223c3d911def4bc2636f3135..59fbbd4747a849df3034423e55c3be171e6ae993 100644
--- a/content/browser/service_worker/service_worker_cache.cc
+++ b/content/browser/service_worker/service_worker_cache.cc
@@ -22,11 +22,20 @@
 namespace content {
 
 namespace {
+struct EntriesDeleter;
 
 typedef scoped_ptr<disk_cache::Backend> ScopedBackendPtr;
 typedef base::Callback<void(bool)> BoolCallback;
 typedef base::Callback<void(disk_cache::ScopedEntryPtr, bool)>
     EntryBoolCallback;
+typedef base::Callback<void(scoped_ptr<ServiceWorkerRequestResponseHeaders>)>
+    HeadersCallback;
+typedef std::vector<disk_cache::Entry*> Entries;
+
+// Automatically closes all of the entries in the vector when it goes out of
+// scope.
+typedef scoped_ptr<Entries, EntriesDeleter> ScopedEntriesPtr;
+
 enum EntryIndex { INDEX_HEADERS = 0, INDEX_RESPONSE_BODY };
 
 // The maximum size of an individual cache. Ultimately cache size is controlled
@@ -36,6 +45,13 @@ const int kMaxCacheBytes = 512 * 1024 * 1024;
 // Buffer size for cache and blob reading/writing.
 const int kBufferSize = 1024 * 512;
 
+struct EntriesDeleter {
+  void operator()(Entries* entries) {
+    for (size_t i = 0, max = entries->size(); i < max; ++i)
+      entries->at(i)->Close();

[michaeln, 2014/08/27 23:45:20]

delete entries too?

[jkarlin, 2014/08/28 15:16:11]

Done.

+  }
+};
+
 struct ResponseReadContext {
   ResponseReadContext(scoped_refptr<net::IOBufferWithSize> buff,
                       scoped_refptr<storage::BlobData> blob)
@@ -189,8 +205,7 @@ void MatchDidReadHeaderData(
     const ServiceWorkerCache::ResponseCallback& callback,
     base::WeakPtr<storage::BlobStorageContext> blob_storage,
     disk_cache::ScopedEntryPtr entry,
-    const scoped_refptr<net::IOBufferWithSize>& buffer,
-    int rv);
+    scoped_ptr<ServiceWorkerRequestResponseHeaders> headers);
 void MatchDidReadResponseBodyData(
     ServiceWorkerFetchRequest* request,
     const ServiceWorkerCache::ResponseCallback& callback,
@@ -211,6 +226,33 @@ void DeleteDidOpenEntry(ServiceWorkerFetchRequest* request,
                         scoped_ptr<disk_cache::Entry*> entryptr,
                         int rv);
 
+// Keys callback
+void KeysDidOpenNextEntry(const ServiceWorkerCache::RequestsCallback& callback,
+                          base::WeakPtr<ServiceWorkerCache> cache,
+                          scoped_ptr<void*> iter,
+                          scoped_ptr<disk_cache::Entry*> entryptr,
+                          ScopedEntriesPtr entries,
+                          int rv);
+void KeysProcessNextEntry(const ServiceWorkerCache::RequestsCallback& callback,
+                          scoped_ptr<ServiceWorkerCache::Requests> requests,
+                          ScopedEntriesPtr entries,
+                          const Entries::iterator& iter);
+void KeysDidReadHeaders(
+    const ServiceWorkerCache::RequestsCallback& callback,
+    scoped_ptr<ServiceWorkerCache::Requests> out_requests,
+    ScopedEntriesPtr entries,
+    const Entries::iterator& iter,
+    scoped_ptr<ServiceWorkerRequestResponseHeaders> headers);
+
+// Copy headers out of a cache entry and into a protobuf. The callback is
+// guaranteed to be run.
+void ReadHeaders(disk_cache::Entry* entry, const HeadersCallback& callback);
+void ReadHeadersDidReadHeaderData(
+    disk_cache::Entry* entry,
+    const HeadersCallback& callback,
+    const scoped_refptr<net::IOBufferWithSize>& buffer,
+    int rv);
+
 // CreateBackend callbacks
 void CreateBackendDidCreate(const ServiceWorkerCache::ErrorCallback& callback,
                             scoped_ptr<ScopedBackendPtr> backend_ptr,
@@ -224,10 +266,8 @@ void PutDidCreateEntry(ServiceWorkerFetchRequest* request,
                        scoped_ptr<storage::BlobDataHandle> blob_data_handle,
                        net::URLRequestContext* request_context,
                        int rv) {
-  if (rv != net::OK) {
-    callback.Run(ServiceWorkerCache::ErrorTypeExists);
-    return;
-  }
+  if (rv != net::OK)
+    return callback.Run(ServiceWorkerCache::ErrorTypeExists);
 
   DCHECK(entryptr);
   disk_cache::ScopedEntryPtr entry(*entryptr);
@@ -258,10 +298,8 @@ void PutDidCreateEntry(ServiceWorkerFetchRequest* request,
   }
 
   scoped_ptr<std::string> serialized(new std::string());
-  if (!headers.SerializeToString(serialized.get())) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage);
-    return;
-  }
+  if (!headers.SerializeToString(serialized.get()))
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage);
 
   scoped_refptr<net::StringIOBuffer> buffer(
       new net::StringIOBuffer(serialized.Pass()));
@@ -298,17 +336,14 @@ void PutDidWriteHeaders(ServiceWorkerResponse* response,
                         int rv) {
   if (rv != expected_bytes) {
     entry->Doom();
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage);
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage);
   }
 
   // The metadata is written, now for the response content. The data is streamed
   // from the blob into the cache entry.
 
-  if (response->blob_uuid.empty()) {
-    callback.Run(ServiceWorkerCache::ErrorTypeOK);
-    return;
-  }
+  if (response->blob_uuid.empty())
+    return callback.Run(ServiceWorkerCache::ErrorTypeOK);
 
   DCHECK(blob_data_handle);
 
@@ -328,8 +363,7 @@ void PutDidWriteBlobToCache(const ServiceWorkerCache::ErrorCallback& callback,
                             bool success) {
   if (!success) {
     entry->Doom();
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage);
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage);
   }
 
   callback.Run(ServiceWorkerCache::ErrorTypeOK);
@@ -341,34 +375,24 @@ void MatchDidOpenEntry(ServiceWorkerFetchRequest* request,
                        scoped_ptr<disk_cache::Entry*> entryptr,
                        int rv) {
   if (rv != net::OK) {
-    callback.Run(ServiceWorkerCache::ErrorTypeNotFound,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeNotFound,
+                        scoped_ptr<ServiceWorkerResponse>(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   DCHECK(entryptr);
   disk_cache::ScopedEntryPtr entry(*entryptr);
 
-  scoped_refptr<net::IOBufferWithSize> buffer(
-      new net::IOBufferWithSize(entry->GetDataSize(INDEX_HEADERS)));
-
   // Copy the entry pointer before passing it in base::Bind.
   disk_cache::Entry* tmp_entry_ptr = entry.get();
 
-  net::CompletionCallback read_header_callback =
-      base::Bind(MatchDidReadHeaderData,
-                 request,
-                 callback,
-                 blob_storage,
-                 base::Passed(entry.Pass()),
-                 buffer);
-
-  int read_rv = tmp_entry_ptr->ReadData(
-      INDEX_HEADERS, 0, buffer.get(), buffer->size(), read_header_callback);
+  HeadersCallback headers_callback = base::Bind(MatchDidReadHeaderData,
+                                                request,
+                                                callback,
+                                                blob_storage,
+                                                base::Passed(entry.Pass()));
 
-  if (read_rv != net::ERR_IO_PENDING)
-    read_header_callback.Run(read_rv);
+  ReadHeaders(tmp_entry_ptr, headers_callback);
 }
 
 void MatchDidReadHeaderData(
@@ -376,55 +400,39 @@ void MatchDidReadHeaderData(
     const ServiceWorkerCache::ResponseCallback& callback,
     base::WeakPtr<storage::BlobStorageContext> blob_storage,
     disk_cache::ScopedEntryPtr entry,
-    const scoped_refptr<net::IOBufferWithSize>& buffer,
-    int rv) {
-  if (rv != buffer->size()) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-
-    return;
-  }
-
-  ServiceWorkerRequestResponseHeaders headers;
-
-  if (!headers.ParseFromArray(buffer->data(), buffer->size())) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-
-    return;
+    scoped_ptr<ServiceWorkerRequestResponseHeaders> headers) {
+  if (!headers) {
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        scoped_ptr<ServiceWorkerResponse>(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   scoped_ptr<ServiceWorkerResponse> response(
       new ServiceWorkerResponse(request->url,
-                                headers.status_code(),
-                                headers.status_text(),
+                                headers->status_code(),
+                                headers->status_text(),
                                 std::map<std::string, std::string>(),
                                 ""));
 
-  for (int i = 0; i < headers.response_headers_size(); ++i) {
+  for (int i = 0; i < headers->response_headers_size(); ++i) {
     const ServiceWorkerRequestResponseHeaders::HeaderMap header =
-        headers.response_headers(i);
+        headers->response_headers(i);
     response->headers.insert(std::make_pair(header.name(), header.value()));
   }
 
   // TODO(jkarlin): Insert vary validation here.
 
   if (entry->GetDataSize(INDEX_RESPONSE_BODY) == 0) {
-    callback.Run(ServiceWorkerCache::ErrorTypeOK,
-                 response.Pass(),
-                 scoped_ptr<storage::BlobDataHandle>());
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeOK,
+                        response.Pass(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   // Stream the response body into a blob.
   if (!blob_storage) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        scoped_ptr<ServiceWorkerResponse>(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   response->blob_uuid = base::GenerateGUID();
@@ -468,10 +476,9 @@ void MatchDidReadResponseBodyData(
     scoped_ptr<ResponseReadContext> response_context,
     int rv) {
   if (rv < 0) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        scoped_ptr<ServiceWorkerResponse>(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   if (rv == 0) {
@@ -519,10 +526,9 @@ void MatchDoneWithBody(ServiceWorkerFetchRequest* request,
                        scoped_ptr<ServiceWorkerResponse> response,
                        scoped_ptr<ResponseReadContext> response_context) {
   if (!blob_storage) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage,
-                 scoped_ptr<ServiceWorkerResponse>(),
-                 scoped_ptr<storage::BlobDataHandle>());
-    return;
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        scoped_ptr<ServiceWorkerResponse>(),
+                        scoped_ptr<storage::BlobDataHandle>());
   }
 
   scoped_ptr<storage::BlobDataHandle> blob_data_handle(
@@ -537,10 +543,8 @@ void DeleteDidOpenEntry(ServiceWorkerFetchRequest* request,
                         const ServiceWorkerCache::ErrorCallback& callback,
                         scoped_ptr<disk_cache::Entry*> entryptr,
                         int rv) {
-  if (rv != net::OK) {
-    callback.Run(ServiceWorkerCache::ErrorTypeNotFound);
-    return;
-  }
+  if (rv != net::OK)
+    return callback.Run(ServiceWorkerCache::ErrorTypeNotFound);
 
   DCHECK(entryptr);
   disk_cache::ScopedEntryPtr entry(*entryptr);
@@ -549,14 +553,135 @@ void DeleteDidOpenEntry(ServiceWorkerFetchRequest* request,
   callback.Run(ServiceWorkerCache::ErrorTypeOK);
 }
 
+void KeysDidOpenNextEntry(const ServiceWorkerCache::RequestsCallback& callback,
+                          base::WeakPtr<ServiceWorkerCache> cache,
+                          scoped_ptr<void*> iter,

[michaeln, 2014/08/27 23:45:20]

might be nice to distinguish the void* iter from the Entries::iterator iter via
naming thruout

[jkarlin, 2014/08/28 15:16:11]

Done.

+                          scoped_ptr<disk_cache::Entry*> entryptr,
+                          ScopedEntriesPtr entries,
+                          int rv) {
+  if (rv == net::ERR_FAILED) {
+    // Enumeration is complete, extract the requests from the entries.
+    Entries::iterator iter = entries->begin();
+    scoped_ptr<ServiceWorkerCache::Requests> out_requests(
+        new ServiceWorkerCache::Requests());

[michaeln, 2014/08/27 23:45:20]

ah ha, here's where the collection to hold the result of the keys method is
cons'd up! you might want to refer to this as out_keys to make that connection
more clear? 

[jkarlin, 2014/08/28 15:16:10]

Done.

+    return KeysProcessNextEntry(

[michaeln, 2014/08/27 23:45:20]

its odd to see return <something> in a function with a void return value? i'd
vote to have return; on the next line instead

[jkarlin, 2014/08/28 15:16:10]

Done for the whole file.

+        callback, out_requests.Pass(), entries.Pass(), iter);
+  }
+
+  if (rv < 0 || !cache) {
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        scoped_ptr<ServiceWorkerCache::Requests>());
+  }
+
+  entries->push_back(*entryptr);
+
+  void** iter_ptr = iter.get();
+  disk_cache::Entry** entry_ptr = entryptr.get();
+
+  net::CompletionCallback open_entry_callback =
+      base::Bind(KeysDidOpenNextEntry,
+                 callback,
+                 cache,
+                 base::Passed(iter.Pass()),
+                 base::Passed(entryptr.Pass()),
+                 base::Passed(entries.Pass()));
+
+  // Enumerate the next entry.
+  int rvv =

[michaeln, 2014/08/27 23:45:20]

looks like you could just use the existing parameter/variable 'rv' here, i was
wondering if rvv was a typo

[jkarlin, 2014/08/28 15:16:11]

Done.

+      cache->backend()->OpenNextEntry(iter_ptr, entry_ptr, open_entry_callback);

[michaeln, 2014/08/27 23:45:20]

i was wondering what caused backend() to be added to the public interface. seems
like this class should more thouroughly encapsulate the backend. if these anon
function where all static instance methods, backend* would not have to be
exposed.

[jkarlin, 2014/08/28 15:16:10]

You're right, better to hide backend.  I'm not gaining any encapsulation with
unnamed functions if I have to expose private variables to do it.  I need to do
the same for a couple other functions later.

+
+  if (rvv != net::ERR_IO_PENDING)
+    open_entry_callback.Run(rvv);
+}
+
+void KeysProcessNextEntry(const ServiceWorkerCache::RequestsCallback& callback,
+                          scoped_ptr<ServiceWorkerCache::Requests> requests,
+                          ScopedEntriesPtr entries,
+                          const Entries::iterator& iter) {
+  if (iter == entries->end())
+    return callback.Run(ServiceWorkerCache::ErrorTypeOK, requests.Pass());

[michaeln, 2014/08/27 23:45:20]

This is the end of the line in the expected case, is that right?

nit: its impressive that the bind mechanism can express ownership and be used to
xfer ownership down the line, but it doesn't always make for the most grokkable
code.

In Keys(), these ptrs to ptrs are created and kept alive thru a very long series
of completion callbacks. 
 830   scoped_ptr<void*> iter(new void*(NULL));
 831   scoped_ptr<disk_cache::Entry*> entry(new disk_cache::Entry*);
 839   ScopedEntriesPtr entries(new Entries());
Additional values are added to the long lived 'state' thats kept alive at key
points in the processing.
 564     Entries::iterator iter = entries->begin();  // not to be confused with
scoped_ptr<void*> iter
 565     scoped_ptr<ServiceWorkerCache::Requests> out_requests(

All the ptr-to-ptr indirection obscures whats going on,
ServiceWorkerCache::Requests is really just a std::vector<T>, and understanding
where objects are created and finally deleted is like hunting for a ghost.
Personal opinion but i think this would be easier to follow and maintain if
there was one struct to bundle up the state needed to process a Keys() method
call and that one thing was new'd at the beginning and Passed down the line.

[jkarlin, 2014/08/28 15:16:11]

Done.

+
+  ReadHeaders(*iter,
+              base::Bind(KeysDidReadHeaders,
+                         callback,
+                         base::Passed(requests.Pass()),
+                         base::Passed(entries.Pass()),
+                         iter));
+}
+
+void KeysDidReadHeaders(
+    const ServiceWorkerCache::RequestsCallback& callback,
+    scoped_ptr<ServiceWorkerCache::Requests> out_requests,
+    ScopedEntriesPtr entries,
+    const Entries::iterator& iter,
+    scoped_ptr<ServiceWorkerRequestResponseHeaders> headers) {
+  disk_cache::Entry* entry = *iter;
+
+  if (!headers) {
+    entry->Doom();

[michaeln, 2014/08/27 23:45:20]

Interesting, a subsequent call to Keys() will yeild a different result since the
iteration will continue at this point to the next item. Would it make sense to
Doom the botched entry here, but to also continue the iteration so subsequent
calls would be expected to get the same results.

[jkarlin, 2014/08/28 15:16:10]

Done.

+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage,
+                        out_requests.Pass());
+  }
+
+  out_requests->push_back(
+      ServiceWorkerFetchRequest(GURL(entry->GetKey()),
+                                headers->method(),
+                                std::map<std::string, std::string>(),
+                                GURL(),
+                                false));
+
+  std::map<std::string, std::string>& req_headers =
+      out_requests->back().headers;
+
+  for (int i = 0; i < headers->request_headers_size(); ++i) {
+    const ServiceWorkerRequestResponseHeaders::HeaderMap header =
+        headers->request_headers(i);
+    req_headers.insert(std::make_pair(header.name(), header.value()));
+  }
+
+  KeysProcessNextEntry(callback, out_requests.Pass(), entries.Pass(), iter + 1);
+}
+
+void ReadHeaders(disk_cache::Entry* entry, const HeadersCallback& callback) {
+  DCHECK(entry);
+
+  scoped_refptr<net::IOBufferWithSize> buffer(
+      new net::IOBufferWithSize(entry->GetDataSize(INDEX_HEADERS)));
+
+  net::CompletionCallback read_header_callback =
+      base::Bind(ReadHeadersDidReadHeaderData, entry, callback, buffer);
+
+  int read_rv = entry->ReadData(
+      INDEX_HEADERS, 0, buffer.get(), buffer->size(), read_header_callback);
+
+  if (read_rv != net::ERR_IO_PENDING)
+    read_header_callback.Run(read_rv);
+}
+
+void ReadHeadersDidReadHeaderData(
+    disk_cache::Entry* entry,
+    const HeadersCallback& callback,
+    const scoped_refptr<net::IOBufferWithSize>& buffer,
+    int rv) {
+  if (rv != buffer->size())
+    return callback.Run(scoped_ptr<ServiceWorkerRequestResponseHeaders>());
+
+  scoped_ptr<ServiceWorkerRequestResponseHeaders> headers(
+      new ServiceWorkerRequestResponseHeaders());
+
+  if (!headers->ParseFromArray(buffer->data(), buffer->size()))
+    return callback.Run(scoped_ptr<ServiceWorkerRequestResponseHeaders>());
+
+  callback.Run(headers.Pass());
+}
+
 void CreateBackendDidCreate(const ServiceWorkerCache::ErrorCallback& callback,
                             scoped_ptr<ScopedBackendPtr> backend_ptr,
                             base::WeakPtr<ServiceWorkerCache> cache,
                             int rv) {
-  if (rv != net::OK || !cache) {
-    callback.Run(ServiceWorkerCache::ErrorTypeStorage);
-    return;
-  }
+  if (rv != net::OK || !cache)
+    return callback.Run(ServiceWorkerCache::ErrorTypeStorage);
+
   cache->set_backend(backend_ptr->Pass());
   callback.Run(ServiceWorkerCache::ErrorTypeOK);
 }
@@ -636,14 +761,12 @@ void ServiceWorkerCache::Put(ServiceWorkerFetchRequest* request,
 
   if (!response->blob_uuid.empty()) {
     if (!blob_storage_context_) {
-      callback.Run(ErrorTypeStorage);
-      return;
+      return callback.Run(ErrorTypeStorage);

[michaeln, 2014/08/27 23:45:20]

why?

[jkarlin, 2014/08/28 15:16:10]

Done.

     }
     blob_data_handle =
         blob_storage_context_->GetBlobDataFromUUID(response->blob_uuid);
     if (!blob_data_handle) {
-      callback.Run(ErrorTypeStorage);
-      return;
+      return callback.Run(ErrorTypeStorage);
     }
   }
 
@@ -701,6 +824,34 @@ void ServiceWorkerCache::Delete(ServiceWorkerFetchRequest* request,
     open_entry_callback.Run(rv);
 }
 
+void ServiceWorkerCache::Keys(const RequestsCallback& callback) {
+  DCHECK(backend_);
+
+  scoped_ptr<void*> iter(new void*(NULL));
+  scoped_ptr<disk_cache::Entry*> entry(new disk_cache::Entry*);
+
+  void** iter_ptr = iter.get();
+  disk_cache::Entry** entry_ptr = entry.get();
+
+  // Load up all of the entries into a vector and then read the request data

[michaeln, 2014/08/27 23:45:20]

It might help to expand on this comment to better explain the sequence of
events, its not so straightforward from a reading of the code.

[jkarlin, 2014/08/28 15:16:10]

Done.

+  // from them. This has to be done in two steps because enumeration breaks if
+  // entries are altered (such as reading header data) while enumerating.

[michaeln, 2014/08/27 23:45:20]

how does reading header data alter it?

[jkarlin, 2014/08/28 15:16:10]

It changes the "last_used" field which seems buggy but the comments clearly warn
that this might happen.  I've clarified the comment.

+  ScopedEntriesPtr entries(new Entries());
+
+  net::CompletionCallback open_entry_callback =
+      base::Bind(KeysDidOpenNextEntry,
+                 callback,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(iter.Pass()),
+                 base::Passed(entry.Pass()),
+                 base::Passed(entries.Pass()));
+
+  int rv = backend_->OpenNextEntry(iter_ptr, entry_ptr, open_entry_callback);
+
+  if (rv != net::ERR_IO_PENDING)
+    open_entry_callback.Run(rv);
+}
+
 bool ServiceWorkerCache::HasCreatedBackend() const {
   return backend_;
 }