Creates CacheContext for ServiceWorkerCacheStorage

content/browser/service_worker/service_worker_cache_storage.cc

Index: content/browser/service_worker/service_worker_cache_storage.cc
diff --git a/content/browser/service_worker/service_worker_cache_storage.cc b/content/browser/service_worker/service_worker_cache_storage.cc
index d7dfbacb9a822fad7f7326c1b497052ca0b17c2f..70eb94d0e3e44c543b8c09545e1cd94e38a1af80 100644
--- a/content/browser/service_worker/service_worker_cache_storage.cc
+++ b/content/browser/service_worker/service_worker_cache_storage.cc
@@ -10,6 +10,7 @@
 #include "base/files/memory_mapped_file.h"
 #include "base/memory/ref_counted.h"
 #include "base/sha1.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "content/browser/service_worker/service_worker_cache.h"
@@ -21,6 +22,19 @@
 
 namespace content {
 
+// The meta information related to each ServiceWorkerCache that the
+// ServiceWorkerCacheManager needs to keep track of.
+// TODO(jkarlin): Add reference counting.

[falken, 2014/08/26 06:31:32]

Depending on how long you plan to take, it might be good to make this TODO more
specific. Something like "so that the deletion of javascript objects can delete
the ServiceWorkerCache" as in the CL description.

[jkarlin, 2014/08/26 12:03:10]

Done.

+struct ServiceWorkerCacheStorage::CacheContext {
+  CacheContext(const std::string& name,
+               CacheID id,
+               scoped_ptr<ServiceWorkerCache> cache)
+      : name(name), id(id), cache(cache.Pass()) {}
+  std::string name;
+  CacheID id;
+  scoped_ptr<ServiceWorkerCache> cache;
+};
+
 // Handles the loading and clean up of ServiceWorkerCache objects.
 class ServiceWorkerCacheStorage::CacheLoader
     : public base::RefCountedThreadSafe<
@@ -53,7 +67,8 @@ class ServiceWorkerCacheStorage::CacheLoader
                                    const BoolCallback& callback) = 0;
 
   // Writes the cache names (and sizes) to disk if applicable.
-  virtual void WriteIndex(CacheMap* caches, const BoolCallback& callback) = 0;
+  virtual void WriteIndex(const CacheMap& caches,
+                          const BoolCallback& callback) = 0;
 
   // Loads the cache names from disk if applicable.
   virtual void LoadIndex(scoped_ptr<std::vector<std::string> > cache_names,
@@ -86,8 +101,7 @@ class ServiceWorkerCacheStorage::MemoryLoader
   virtual void CreateCache(const std::string& cache_name,
                            const CacheCallback& callback) OVERRIDE {
     scoped_ptr<ServiceWorkerCache> cache =
-        ServiceWorkerCache::CreateMemoryCache(
-            cache_name, request_context_, blob_context_);
+        ServiceWorkerCache::CreateMemoryCache(request_context_, blob_context_);
     callback.Run(cache.Pass());
   }
 
@@ -96,7 +110,7 @@ class ServiceWorkerCacheStorage::MemoryLoader
     callback.Run(true);
   }
 
-  virtual void WriteIndex(CacheMap* caches,
+  virtual void WriteIndex(const CacheMap& caches,
                           const BoolCallback& callback) OVERRIDE {
     callback.Run(false);
   }
@@ -167,7 +181,6 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
     scoped_ptr<ServiceWorkerCache> cache =
         ServiceWorkerCache::CreatePersistentCache(
             CreatePersistentCachePath(origin_path_, cache_name),
-            cache_name,
             request_context_,
             blob_context_);
     callback.Run(cache.Pass());
@@ -243,7 +256,7 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
     original_loop->PostTask(FROM_HERE, base::Bind(callback, rv));
   }
 
-  virtual void WriteIndex(CacheMap* caches,
+  virtual void WriteIndex(const CacheMap& caches,
                           const BoolCallback& callback) OVERRIDE {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -252,11 +265,11 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
 
     ServiceWorkerCacheStorageIndex index;
 
-    for (CacheMap::const_iterator iter(caches); !iter.IsAtEnd();
-         iter.Advance()) {
-      const ServiceWorkerCache* cache = iter.GetCurrentValue();
+    for (CacheMap::const_iterator it = caches.begin(); it != caches.end();
+         ++it) {
+      const CacheContext* cache = it->second;
       ServiceWorkerCacheStorageIndex::Cache* index_cache = index.add_cache();
-      index_cache->set_name(cache->name());
+      index_cache->set_name(cache->name);
       index_cache->set_size(0);  // TODO(jkarlin): Make this real.
     }
 
@@ -274,7 +287,6 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
                    tmp_path,
                    index_path,
                    serialized,
-                   caches,
                    callback,
                    base::MessageLoopProxy::current()));
   }
@@ -283,7 +295,6 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
       const base::FilePath& tmp_path,
       const base::FilePath& index_path,
       const std::string& data,
-      CacheMap* caches,
       const BoolCallback& callback,
       const scoped_refptr<base::MessageLoopProxy>& original_loop) {
     DCHECK(cache_task_runner_->RunsTasksOnCurrentThread());
@@ -379,6 +390,7 @@ ServiceWorkerCacheStorage::ServiceWorkerCacheStorage(
     net::URLRequestContext* request_context,
     base::WeakPtr<storage::BlobStorageContext> blob_context)
     : initialized_(false),
+      next_cache_id_(0),
       origin_path_(path),
       cache_task_runner_(cache_task_runner),
       weak_factory_(this) {
@@ -391,6 +403,7 @@ ServiceWorkerCacheStorage::ServiceWorkerCacheStorage(
 }
 
 ServiceWorkerCacheStorage::~ServiceWorkerCacheStorage() {
+  STLDeleteContainerPairSecondPointers(cache_map_.begin(), cache_map_.end());
 }
 
 void ServiceWorkerCacheStorage::CreateCache(
@@ -405,12 +418,12 @@ void ServiceWorkerCacheStorage::CreateCache(
   }
 
   if (cache_name.empty()) {
-    callback.Run(0, CACHE_STORAGE_ERROR_EMPTY_KEY);
+    callback.Run(-1, CACHE_STORAGE_ERROR_EMPTY_KEY);

[falken, 2014/08/26 06:31:33]

Should we have a kInvalidCacheId constant?

[jkarlin, 2014/08/26 12:03:10]

Done. Now used in the tests as well.

     return;
   }
 
   if (GetLoadedCache(cache_name)) {
-    callback.Run(0, CACHE_STORAGE_ERROR_EXISTS);
+    callback.Run(-1, CACHE_STORAGE_ERROR_EXISTS);
     return;
   }
 
@@ -436,19 +449,21 @@ void ServiceWorkerCacheStorage::GetCache(
   }
 
   if (cache_name.empty()) {
-    callback.Run(0, CACHE_STORAGE_ERROR_EMPTY_KEY);
+    callback.Run(-1, CACHE_STORAGE_ERROR_EMPTY_KEY);
     return;
   }
 
-  ServiceWorkerCache* cache = GetLoadedCache(cache_name);
-  if (!cache) {
-    callback.Run(0, CACHE_STORAGE_ERROR_NOT_FOUND);
+  CacheContext* cache_context = GetLoadedCache(cache_name);
+  if (!cache_context) {
+    callback.Run(-1, CACHE_STORAGE_ERROR_NOT_FOUND);
     return;
   }
 
+  ServiceWorkerCache* cache = cache_context->cache.get();
   cache->CreateBackend(base::Bind(&ServiceWorkerCacheStorage::DidCreateBackend,
                                   weak_factory_.GetWeakPtr(),
                                   cache->AsWeakPtr(),
+                                  cache_context->id,
                                   callback));
 }
 
@@ -492,18 +507,18 @@ void ServiceWorkerCacheStorage::DeleteCache(
     return;
   }
 
-  ServiceWorkerCache* cache = GetLoadedCache(cache_name);
-  if (!cache) {
+  scoped_ptr<CacheContext> cache_context(GetLoadedCache(cache_name));

[falken, 2014/08/26 06:31:32]

It's a bit subtle that the raw pointer returned by GetLoadedCache is wrapped in
a scoped_ptr specifically to delete it. Maybe add a brief comment?

[jkarlin, 2014/08/26 12:03:10]

Agree.  Specifically call .reset() now to make it clear.

+  if (!cache_context) {
     callback.Run(false, CACHE_STORAGE_ERROR_NOT_FOUND);
     return;
   }
 
   name_map_.erase(cache_name);
-  cache_map_.Remove(cache->id());  // deletes cache
+  cache_map_.erase(cache_context->id);
 
   // Update the Index
   cache_loader_->WriteIndex(
-      &cache_map_,
+      cache_map_,
       base::Bind(&ServiceWorkerCacheStorage::DeleteCacheDidWriteIndex,
                  weak_factory_.GetWeakPtr(),
                  cache_name,
@@ -532,6 +547,7 @@ void ServiceWorkerCacheStorage::EnumerateCaches(
 
 void ServiceWorkerCacheStorage::DidCreateBackend(
     base::WeakPtr<ServiceWorkerCache> cache,
+    CacheID cache_id,
     const CacheAndErrorCallback& callback,
     bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -539,10 +555,10 @@ void ServiceWorkerCacheStorage::DidCreateBackend(
   if (!success || !cache) {
     // TODO(jkarlin): This should delete the directory and try again in case
     // the cache is simply corrupt.
-    callback.Run(0, CACHE_STORAGE_ERROR_STORAGE);
+    callback.Run(-1, CACHE_STORAGE_ERROR_STORAGE);
     return;
   }
-  callback.Run(cache->id(), CACHE_STORAGE_ERROR_NO_ERROR);
+  callback.Run(cache_id, CACHE_STORAGE_ERROR_NO_ERROR);
 }
 
 // Init is run lazily so that it is called on the proper MessageLoop.
@@ -591,18 +607,20 @@ void ServiceWorkerCacheStorage::LazyInitDidLoadIndex(
                  weak_factory_.GetWeakPtr(),
                  callback,
                  base::Passed(indexed_cache_names.Pass()),
-                 iter_next));
+                 iter_next,
+                 *iter));
 }
 
 void ServiceWorkerCacheStorage::LazyInitIterateAndLoadCacheName(
     const base::Closure& callback,
     scoped_ptr<std::vector<std::string> > indexed_cache_names,
     const std::vector<std::string>::const_iterator& iter,
+    const std::string& cache_name,
     scoped_ptr<ServiceWorkerCache> cache) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (cache)
-    AddCacheToMaps(cache.Pass());
+    AddCacheToMaps(cache_name, cache.Pass());
 
   if (iter == indexed_cache_names->end()) {
     LazyInitDone();
@@ -616,7 +634,8 @@ void ServiceWorkerCacheStorage::LazyInitIterateAndLoadCacheName(
                  weak_factory_.GetWeakPtr(),
                  callback,
                  base::Passed(indexed_cache_names.Pass()),
-                 iter_next));
+                 iter_next,
+                 *iter));
 }
 
 void ServiceWorkerCacheStorage::LazyInitDone() {
@@ -629,14 +648,18 @@ void ServiceWorkerCacheStorage::LazyInitDone() {
   init_callbacks_.clear();
 }
 
-void ServiceWorkerCacheStorage::AddCacheToMaps(
-    scoped_ptr<ServiceWorkerCache> cache) {
+ServiceWorkerCacheStorage::CacheContext*
+ServiceWorkerCacheStorage::AddCacheToMaps(
+    const std::string& cache_name,
+    scoped_ptr<ServiceWorkerCache> sw_cache) {

[falken, 2014/08/26 06:31:32]

nit: Why rename? The other ServiceWorkerCache objects in this file are just
"cache".

[jkarlin, 2014/08/26 12:03:10]

Done.

   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  ServiceWorkerCache* cache_ptr = cache.release();
-  CacheID id = cache_map_.Add(cache_ptr);
-  name_map_.insert(std::make_pair(cache_ptr->name(), id));
-  cache_ptr->set_id(id);
+  CacheID id = next_cache_id_++;
+  CacheContext* cache_context =
+      new CacheContext(cache_name, id, sw_cache.Pass());
+  cache_map_.insert(std::make_pair(id, cache_context));  // Takes ownership
+  name_map_.insert(std::make_pair(cache_name, id));
+  return cache_context;
 }
 
 void ServiceWorkerCacheStorage::CreateCacheDidCreateCache(
@@ -646,35 +669,35 @@ void ServiceWorkerCacheStorage::CreateCacheDidCreateCache(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!cache) {
-    callback.Run(0, CACHE_STORAGE_ERROR_STORAGE);
+    callback.Run(-1, CACHE_STORAGE_ERROR_CLOSING);
     return;
   }
 
-  base::WeakPtr<ServiceWorkerCache> cache_ptr = cache->AsWeakPtr();
-
-  AddCacheToMaps(cache.Pass());
+  CacheContext* cache_context = AddCacheToMaps(cache_name, cache.Pass());
 
   cache_loader_->WriteIndex(
-      &cache_map_,
-      base::Bind(
-          &ServiceWorkerCacheStorage::CreateCacheDidWriteIndex,
-          weak_factory_.GetWeakPtr(),
-          callback,
-          cache_ptr));  // cache is owned by this->CacheMap and won't be deleted
+      cache_map_,
+      base::Bind(&ServiceWorkerCacheStorage::CreateCacheDidWriteIndex,
+                 weak_factory_.GetWeakPtr(),
+                 callback,
+                 cache_context->cache->AsWeakPtr(),
+                 cache_context->id));
 }
 
 void ServiceWorkerCacheStorage::CreateCacheDidWriteIndex(
     const CacheAndErrorCallback& callback,
     base::WeakPtr<ServiceWorkerCache> cache,
+    CacheID id,
     bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!cache) {
-    callback.Run(false, CACHE_STORAGE_ERROR_STORAGE);
+    callback.Run(false, CACHE_STORAGE_ERROR_CLOSING);
     return;
   }
   cache->CreateBackend(base::Bind(&ServiceWorkerCacheStorage::DidCreateBackend,
                                   weak_factory_.GetWeakPtr(),
                                   cache,
+                                  id,
                                   callback));
 }
 
@@ -699,18 +722,18 @@ void ServiceWorkerCacheStorage::DeleteCacheDidCleanUp(
   callback.Run(true, CACHE_STORAGE_ERROR_NO_ERROR);
 }
 
-ServiceWorkerCache* ServiceWorkerCacheStorage::GetLoadedCache(
-    const std::string& cache_name) const {
+ServiceWorkerCacheStorage::CacheContext*
+ServiceWorkerCacheStorage::GetLoadedCache(const std::string& cache_name) const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(initialized_);
 
-  NameMap::const_iterator it = name_map_.find(cache_name);
-  if (it == name_map_.end())
+  NameMap::const_iterator name_iter = name_map_.find(cache_name);
+  if (name_iter == name_map_.end())
     return NULL;
 
-  ServiceWorkerCache* cache = cache_map_.Lookup(it->second);
-  DCHECK(cache);
-  return cache;
+  CacheMap::const_iterator map_iter = cache_map_.find(name_iter->second);
+  DCHECK(map_iter != cache_map_.end());
+  return map_iter->second;
 }
 
 }  // namespace content