[CacheStorage] Pad and bin opaque resource sizes.

content/browser/cache_storage/cache_storage_manager_unittest.cc

Index: content/browser/cache_storage/cache_storage_manager_unittest.cc
diff --git a/content/browser/cache_storage/cache_storage_manager_unittest.cc b/content/browser/cache_storage/cache_storage_manager_unittest.cc
index 72afefa6028c2c63d255c6a9226881af7d5ac425..cbf94d095192db3dd64974181237194aa8946a0d 100644
--- a/content/browser/cache_storage/cache_storage_manager_unittest.cc
+++ b/content/browser/cache_storage/cache_storage_manager_unittest.cc
@@ -46,6 +46,8 @@
 #include "storage/browser/test/mock_special_storage_policy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using blink::mojom::FetchResponseType;
+
 namespace content {
 
 namespace {
@@ -347,25 +349,31 @@ class CacheStorageManagerTest : public testing::Test {
     return callback_error_ == CACHE_STORAGE_OK;
   }
 
-  bool CachePut(CacheStorageCache* cache, const GURL& url) {
+  bool CachePut(CacheStorageCache* cache,
+                const GURL& url,
+                FetchResponseType response_type = FetchResponseType::kDefault) {
     ServiceWorkerFetchRequest request;
     request.url = url;
 
-    return CachePutWithStatusCode(cache, request, 200);
+    return CachePutWithStatusCode(cache, request, 200, response_type);
   }
 
   bool CachePutWithRequestAndHeaders(
       CacheStorageCache* cache,
       const ServiceWorkerFetchRequest& request,
-      const ServiceWorkerHeaderMap& response_headers) {
-    return CachePutWithStatusCode(cache, request, 200, response_headers);
+      const ServiceWorkerHeaderMap& response_headers,
+      FetchResponseType response_type = FetchResponseType::kDefault) {
+    return CachePutWithStatusCode(cache, request, 200, response_type,
+                                  response_headers);
   }
 
-  bool CachePutWithStatusCode(CacheStorageCache* cache,
-                              const ServiceWorkerFetchRequest& request,
-                              int status_code,
-                              const ServiceWorkerHeaderMap& response_headers =
-                                  ServiceWorkerHeaderMap()) {
+  bool CachePutWithStatusCode(
+      CacheStorageCache* cache,
+      const ServiceWorkerFetchRequest& request,
+      int status_code,
+      FetchResponseType response_type = FetchResponseType::kDefault,
+      const ServiceWorkerHeaderMap& response_headers =
+          ServiceWorkerHeaderMap()) {
     std::unique_ptr<storage::BlobDataBuilder> blob_data(
         new storage::BlobDataBuilder(base::GenerateGUID()));
     blob_data->AppendData(request.url.spec());
@@ -376,8 +384,7 @@ class CacheStorageManagerTest : public testing::Test {
         base::MakeUnique<std::vector<GURL>>();
     url_list->push_back(request.url);
     ServiceWorkerResponse response(
-        std::move(url_list), status_code, "OK",
-        blink::mojom::FetchResponseType::kDefault,
+        std::move(url_list), status_code, "OK", response_type,
         base::MakeUnique<ServiceWorkerHeaderMap>(response_headers),
         blob_handle->uuid(), request.url.spec().size(),
         blink::kWebServiceWorkerResponseErrorUnknown, base::Time(),
@@ -468,6 +475,28 @@ class CacheStorageManagerTest : public testing::Test {
     return callback_usage_;
   }
 
+  int64_t GetQuotaOriginUsage(const GURL& origin) {
+    int64_t usage(CacheStorage::kSizeUnknown);
+    base::RunLoop loop;
+    quota_manager_proxy_->GetUsageAndQuota(
+        base::ThreadTaskRunnerHandle::Get().get(), origin,
+        StorageType::kStorageTypeTemporary,
+        base::Bind(&CacheStorageManagerTest::DidGetQuotaOriginUsage,
+                   base::Unretained(this), base::Unretained(&usage), &loop));
+    loop.Run();
+    return usage;
+  }
+
+  void DidGetQuotaOriginUsage(int64_t* out_usage,
+                              base::RunLoop* run_loop,
+                              QuotaStatusCode status_code,
+                              int64_t usage,
+                              int64_t quota) {
+    if (status_code == storage::kQuotaStatusOk)
+      *out_usage = usage;
+    run_loop->Quit();
+  }
+
  protected:
   // Temporary directory must be allocated first so as to be destroyed last.
   base::ScopedTempDir temp_dir_;
@@ -917,6 +946,107 @@ TEST_F(CacheStorageManagerTest, CacheSizeCorrectAfterReopen) {
   EXPECT_EQ(size_before_close, Size(origin1_));
 }
 
+TEST_F(CacheStorageManagerTest, CacheSizePaddedAfterReopen) {
+  const GURL kFooURL = origin1_.Resolve("foo");
+  const std::string kCacheName = "foo";
+
+  int64_t put_delta = quota_manager_proxy_->last_notified_delta();
+  EXPECT_EQ(0, put_delta);
+  EXPECT_EQ(0, quota_manager_proxy_->notify_storage_modified_count());
+
+  EXPECT_TRUE(Open(origin1_, kCacheName));
+  std::unique_ptr<CacheStorageCacheHandle> original_handle =
+      std::move(callback_cache_handle_);
+
+  base::RunLoop().RunUntilIdle();
+  put_delta += quota_manager_proxy_->last_notified_delta();
+  EXPECT_EQ(0, put_delta);
+  EXPECT_EQ(0, quota_manager_proxy_->notify_storage_modified_count());
+
+  EXPECT_TRUE(
+      CachePut(original_handle->value(), kFooURL, FetchResponseType::kOpaque));
+  int64_t cache_size_before_close = Size(origin1_);
+  base::FilePath storage_dir = original_handle->value()->path().DirName();
+  original_handle = nullptr;
+  EXPECT_GT(cache_size_before_close, 0);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(GetQuotaOriginUsage(origin1_), cache_size_before_close);

[jkarlin, 2017/07/31 14:03:25]

swap argument order

[cmumford, 2017/08/10 16:37:11]

Done.

+
+  base::RunLoop().RunUntilIdle();
+  put_delta = quota_manager_proxy_->last_notified_delta();
+  EXPECT_GT(put_delta, 0);
+  EXPECT_EQ(1, quota_manager_proxy_->notify_storage_modified_count());
+
+  EXPECT_EQ(GetQuotaOriginUsage(origin1_), put_delta);
+
+  // Close the caches and cache manager.
+  EXPECT_TRUE(FlushCacheStorageIndex(origin1_));
+  DestroyStorageManager();
+
+  // Create a new CacheStorageManager that hasn't yet loaded the origin.
+  CreateStorageManager();
+  quota_manager_proxy_->SimulateQuotaManagerDestroyed();
+  cache_manager_ = CacheStorageManager::Create(cache_manager_.get());
+  EXPECT_TRUE(Open(origin1_, kCacheName));
+
+  base::RunLoop().RunUntilIdle();
+  put_delta = quota_manager_proxy_->last_notified_delta();
+  EXPECT_EQ(0, put_delta);
+  EXPECT_EQ(0, quota_manager_proxy_->notify_storage_modified_count());
+
+  EXPECT_EQ(cache_size_before_close, Size(origin1_));
+}
+
+TEST_F(CacheStorageManagerTest, PersistedCacheKeyUsed) {
+  const GURL kFooURL = origin1_.Resolve("foo");
+  const std::string kCacheName = "foo";
+
+  EXPECT_TRUE(Open(origin1_, kCacheName));
+  std::unique_ptr<CacheStorageCacheHandle> original_handle =
+      std::move(callback_cache_handle_);
+
+  EXPECT_TRUE(
+      CachePut(original_handle->value(), kFooURL, FetchResponseType::kOpaque));
+
+  int64_t cache_size_after_put = Size(origin1_);
+  EXPECT_GT(cache_size_after_put, 0);

[jkarlin, 2017/07/31 14:03:25]

swap argument order

[cmumford, 2017/08/10 16:37:11]

Are you sure? EXPECT_GT(arg1, arg2) is expecting that arg1 > arg2 right?

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

[jkarlin, 2017/08/11 15:04:37]

Sorry. What I'm looking for is to have the expected value as the first argument,
so:

EXPECT_LT(0, cache_size_after_put);

+
+  // Close the caches and cache manager.
+  EXPECT_TRUE(FlushCacheStorageIndex(origin1_));
+  DestroyStorageManager();
+
+  // GenerateNewKeyForTest isn't thread safe so
+  base::RunLoop().RunUntilIdle();
+  CacheStorage::GenerateNewKeyForTesting();
+
+  // Create a new CacheStorageManager that hasn't yet loaded the origin.
+  CreateStorageManager();
+  quota_manager_proxy_->SimulateQuotaManagerDestroyed();
+  cache_manager_ = CacheStorageManager::Create(cache_manager_.get());
+
+  // Reopening the origin/cache creates a new CacheStorage instance with a new
+  // random key.
+  EXPECT_TRUE(Open(origin1_, kCacheName));
+
+  // Size (before any change) should be the same as before it was closed.
+  EXPECT_EQ(Size(origin1_), cache_size_after_put);

[jkarlin, 2017/07/31 14:03:25]

swap argument order

[cmumford, 2017/08/10 16:37:11]

Done.

+
+  // Delete the value. If the new padding key was used to deduct the padded size
+  // then after deletion we would expect to see a non-zero cache size.
+  EXPECT_TRUE(Delete(origin1_, "foo"));
+  EXPECT_EQ(Size(origin1_), 0);

[jkarlin, 2017/07/31 14:03:25]

swap argument order

+
+  // Now put the exact same resource back into the cache. This time we expect to
+  // see a different size as the padding is calculated with a different key.
+  std::unique_ptr<CacheStorageCacheHandle> new_handle =
+      std::move(callback_cache_handle_);
+  EXPECT_TRUE(
+      CachePut(new_handle->value(), kFooURL, FetchResponseType::kOpaque));
+
+  EXPECT_NE(Size(origin1_), cache_size_after_put);

[jkarlin, 2017/07/31 14:03:25]

swap argument order

[cmumford, 2017/08/10 16:37:11]

Done, but I don't think that either of these are "expected" arguments are they?

[jkarlin, 2017/08/11 15:04:37]

cache_size_after_put is the thing that is known (via previous checks) to be
correct, and Size(origin1_) is what's being tested here.

+}
+
 // With a memory cache the cache can't be freed from memory until the client
 // calls delete.
 TEST_F(CacheStorageManagerMemoryOnlyTest, MemoryLosesReferenceOnlyAfterDelete) {