Sort ServiceWorkerCacheStorage::Keys() in creation order.

Keeps a vector of cache names in creation order.  They're written to and read from the the index file in the same order.

This simplifies the keys unit tests as the order of the vectors is always the same and they can now be compared directly.

BUG=408787

Review URL: https://codereview.chromium.org/565913002

Cr-Commit-Position: refs/heads/master@{#294838}

content/browser/service_worker/service_worker_cache_storage.cc

@@ -31,7 +31,7 @@ class ServiceWorkerCacheStorage::CacheLoader {
       CacheCallback;
   typedef base::Callback<void(bool)> BoolCallback;
   typedef base::Callback<void(scoped_ptr<std::vector<std::string> >)>
-      StringsCallback;
+      StringVectorCallback;
 
   CacheLoader(base::SequencedTaskRunner* cache_task_runner,
               net::URLRequestContext* request_context,
@@ -57,12 +57,12 @@ class ServiceWorkerCacheStorage::CacheLoader {
                                    const BoolCallback& callback) = 0;
 
   // Writes the cache names (and sizes) to disk if applicable.
-  virtual void WriteIndex(const CacheMap& caches,
+  virtual void WriteIndex(const StringVector& cache_names,
                           const BoolCallback& callback) = 0;
 
   // Loads the cache names from disk if applicable.
   virtual void LoadIndex(scoped_ptr<std::vector<std::string> > cache_names,
-                         const StringsCallback& callback) = 0;
+                         const StringVectorCallback& callback) = 0;
 
  protected:
   scoped_refptr<base::SequencedTaskRunner> cache_task_runner_;
@@ -104,13 +104,13 @@ class ServiceWorkerCacheStorage::MemoryLoader
     callback.Run(true);
   }
 
-  virtual void WriteIndex(const CacheMap& caches,
+  virtual void WriteIndex(const StringVector& cache_names,
                           const BoolCallback& callback) OVERRIDE {
     callback.Run(false);
   }
 
   virtual void LoadIndex(scoped_ptr<std::vector<std::string> > cache_names,
-                         const StringsCallback& callback) OVERRIDE {
+                         const StringVectorCallback& callback) OVERRIDE {
     callback.Run(cache_names.Pass());
   }
 
@@ -208,7 +208,7 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
     original_loop->PostTask(FROM_HERE, base::Bind(callback, rv));
   }
 
-  virtual void WriteIndex(const CacheMap& caches,
+  virtual void WriteIndex(const StringVector& cache_names,
                           const BoolCallback& callback) OVERRIDE {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -217,10 +217,9 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
 
     ServiceWorkerCacheStorageIndex index;
 
-    for (CacheMap::const_iterator it = caches.begin(); it != caches.end();
-         ++it) {
+    for (size_t i = 0u, max = cache_names.size(); i < max; ++i) {
       ServiceWorkerCacheStorageIndex::Cache* index_cache = index.add_cache();
-      index_cache->set_name(it->first);
+      index_cache->set_name(cache_names[i]);
       index_cache->set_size(0);  // TODO(jkarlin): Make this real.
     }
 
@@ -259,7 +258,7 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
   }
 
   virtual void LoadIndex(scoped_ptr<std::vector<std::string> > names,
-                         const StringsCallback& callback) OVERRIDE {
+                         const StringVectorCallback& callback) OVERRIDE {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
     // 1. Read the file from disk. (LoadIndexReadFileInPool)
@@ -279,7 +278,7 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
   static void LoadIndexReadFileInPool(
       const base::FilePath& index_path,
       scoped_ptr<std::vector<std::string> > names,
-      const StringsCallback& callback,
+      const StringVectorCallback& callback,
       const scoped_refptr<base::MessageLoopProxy>& original_loop) {
     std::string body;
     base::ReadFileToString(index_path, &body);
@@ -292,7 +291,7 @@ class ServiceWorkerCacheStorage::SimpleCacheLoader
   }
 
   static void LoadIndexDidReadFile(scoped_ptr<std::vector<std::string> > names,
-                                   const StringsCallback& callback,
+                                   const StringVectorCallback& callback,
                                    const std::string& serialized) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -442,9 +441,15 @@ void ServiceWorkerCacheStorage::DeleteCache(
 
   cache_map_.erase(it);
 
+  // Delete the name from ordered_cache_names_.
+  StringVector::iterator iter = std::find(
+      ordered_cache_names_.begin(), ordered_cache_names_.end(), cache_name);
+  DCHECK(iter != ordered_cache_names_.end());
+  ordered_cache_names_.erase(iter);
+
   // Update the Index
   cache_loader_->WriteIndex(
-      cache_map_,
+      ordered_cache_names_,
       base::Bind(&ServiceWorkerCacheStorage::DeleteCacheDidWriteIndex,
                  weak_factory_.GetWeakPtr(),
                  cache_name,
@@ -462,13 +467,7 @@ void ServiceWorkerCacheStorage::EnumerateCaches(
     return;
   }
 
-  std::vector<std::string> names;
-  for (CacheMap::const_iterator it = cache_map_.begin(); it != cache_map_.end();
-       ++it) {
-    names.push_back(it->first);
-  }
-
-  callback.Run(names, CACHE_STORAGE_ERROR_NO_ERROR);
+  callback.Run(ordered_cache_names_, CACHE_STORAGE_ERROR_NO_ERROR);
 }
 
 // Init is run lazily so that it is called on the proper MessageLoop.
@@ -506,6 +505,7 @@ void ServiceWorkerCacheStorage::LazyInitDidLoadIndex(
   for (size_t i = 0u, max = indexed_cache_names->size(); i < max; ++i) {
     cache_map_.insert(std::make_pair(indexed_cache_names->at(i),
                                      base::WeakPtr<ServiceWorkerCache>()));
+    ordered_cache_names_.push_back(indexed_cache_names->at(i));
   }
 
   initialized_ = true;
@@ -530,9 +530,10 @@ void ServiceWorkerCacheStorage::CreateCacheDidCreateCache(
   }
 
   cache_map_.insert(std::make_pair(cache_name, cache->AsWeakPtr()));
+  ordered_cache_names_.push_back(cache_name);
 
   cache_loader_->WriteIndex(
-      cache_map_,
+      ordered_cache_names_,
       base::Bind(&ServiceWorkerCacheStorage::CreateCacheDidWriteIndex,
                  weak_factory_.GetWeakPtr(),
                  callback,

content/browser/service_worker/service_worker_cache_storage.h

@@ -42,12 +42,12 @@ class CONTENT_EXPORT ServiceWorkerCacheStorage {
     CACHE_STORAGE_ERROR_STORAGE,
     CACHE_STORAGE_ERROR_CLOSING
   };
-
+  typedef std::vector<std::string> StringVector;
   typedef base::Callback<void(bool, CacheStorageError)> BoolAndErrorCallback;
   typedef base::Callback<void(const scoped_refptr<ServiceWorkerCache>&,
                               CacheStorageError)> CacheAndErrorCallback;
-  typedef base::Callback<void(const std::vector<std::string>&,
-                              CacheStorageError)> StringsAndErrorCallback;
+  typedef base::Callback<void(const StringVector&, CacheStorageError)>
+      StringsAndErrorCallback;
 
   ServiceWorkerCacheStorage(
       const base::FilePath& origin_path,
@@ -95,7 +95,8 @@ class CONTENT_EXPORT ServiceWorkerCacheStorage {
   scoped_refptr<ServiceWorkerCache> GetLoadedCache(
       const std::string& cache_name);
 
-  // Initializer and its callback are below.
+  // Initializer and its callback are below. While LazyInit is running any new
+  // operations will be queued and started in order after initialization.
   void LazyInit(const base::Closure& closure);
   void LazyInitDidLoadIndex(
       const base::Closure& callback,
@@ -129,6 +130,9 @@ class CONTENT_EXPORT ServiceWorkerCacheStorage {
   // The map of cache names to ServiceWorkerCache objects.
   CacheMap cache_map_;
 
+  // The names of caches in the order that they were created.
+  StringVector ordered_cache_names_;
+
   // The file path for this CacheStorage.
   base::FilePath origin_path_;
 

content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc

@@ -223,21 +223,6 @@ class ServiceWorkerCacheStorageManagerTest : public testing::Test {
     return !error;
   }
 
-  bool VerifyKeys(const std::vector<std::string>& expected_keys) {
-    if (expected_keys.size() != callback_strings_.size())
-      return false;
-
-    std::set<std::string> found_set;
-    for (int i = 0, max = callback_strings_.size(); i < max; ++i)
-      found_set.insert(callback_strings_[i]);
-
-    for (int i = 0, max = expected_keys.size(); i < max; ++i) {
-      if (found_set.find(expected_keys[i]) == found_set.end())
-        return false;
-    }
-    return true;
-  }
-
  protected:
   TestBrowserContext browser_context_;
   TestBrowserThreadBundle browser_thread_bundle_;
@@ -355,7 +340,7 @@ TEST_P(ServiceWorkerCacheStorageManagerTestP, SomeKeys) {
   std::vector<std::string> expected_keys;
   expected_keys.push_back("foo");
   expected_keys.push_back("bar");
-  EXPECT_TRUE(VerifyKeys(expected_keys));
+  EXPECT_TRUE(expected_keys == callback_strings_);
   EXPECT_TRUE(Keys(origin2_));
   EXPECT_EQ(1u, callback_strings_.size());
   EXPECT_STREQ("baz", callback_strings_[0].c_str());
@@ -408,7 +393,7 @@ TEST_F(ServiceWorkerCacheStorageManagerTest, DataPersists) {
   std::vector<std::string> expected_keys;
   expected_keys.push_back("foo");
   expected_keys.push_back("baz");
-  EXPECT_TRUE(VerifyKeys(expected_keys));
+  EXPECT_TRUE(expected_keys == callback_strings_);
 }
 
 TEST_F(ServiceWorkerCacheStorageManagerMemoryOnlyTest, DataLostWhenMemoryOnly) {