CacheStorage: Support multiple batch operations

content/browser/cache_storage/cache_storage_cache_unittest.cc

Index: content/browser/cache_storage/cache_storage_cache_unittest.cc
diff --git a/content/browser/cache_storage/cache_storage_cache_unittest.cc b/content/browser/cache_storage/cache_storage_cache_unittest.cc
index 3da11ec97fe250445b9e41bccc00bbfcc145d481..7e7da7b59bfabc3a646459d8d394013de38d6098 100644
--- a/content/browser/cache_storage/cache_storage_cache_unittest.cc
+++ b/content/browser/cache_storage/cache_storage_cache_unittest.cc
@@ -252,21 +252,12 @@ class CacheStorageCacheTest : public testing::Test {
         request.is_reload));
   }
 
-  scoped_ptr<ServiceWorkerResponse> CopyFetchResponse(
-      const ServiceWorkerResponse& response) {
-    scoped_ptr<ServiceWorkerResponse> sw_response(new ServiceWorkerResponse(
-        response.url, response.status_code, response.status_text,
-        response.response_type, response.headers, response.blob_uuid,
-        response.blob_size, response.stream_url));
-    return sw_response.Pass();
-  }
-
-  bool Put(const ServiceWorkerFetchRequest& request,
-           const ServiceWorkerResponse& response) {
+  CacheStorageCache::ErrorType BatchOperation(
+      const std::vector<CacheStorageBatchOperation>& operations) {
     scoped_ptr<base::RunLoop> loop(new base::RunLoop());
 
-    cache_->Put(
-        CopyFetchRequest(request), CopyFetchResponse(response),
+    cache_->BatchOperation(
+        operations,
         base::Bind(&CacheStorageCacheTest::ErrorTypeCallback,
                    base::Unretained(this), base::Unretained(loop.get())));
     // TODO(jkarlin): These functions should use base::RunLoop().RunUntilIdle()
@@ -274,7 +265,19 @@ class CacheStorageCacheTest : public testing::Test {
     // thread.
     loop->Run();
 
-    return callback_error_ == CacheStorageCache::ERROR_TYPE_OK;
+    return callback_error_;
+  }
+
+  bool Put(const ServiceWorkerFetchRequest& request,
+           const ServiceWorkerResponse& response) {
+    CacheStorageBatchOperation operation;
+    operation.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+    operation.request = request;
+    operation.response = response;
+
+    CacheStorageCache::ErrorType error =
+        BatchOperation(std::vector<CacheStorageBatchOperation>(1, operation));
+    return error == CacheStorageCache::ERROR_TYPE_OK;
   }
 
   bool Match(const ServiceWorkerFetchRequest& request) {
@@ -290,15 +293,13 @@ class CacheStorageCacheTest : public testing::Test {
   }
 
   bool Delete(const ServiceWorkerFetchRequest& request) {
-    scoped_ptr<base::RunLoop> loop(new base::RunLoop());
+    CacheStorageBatchOperation operation;
+    operation.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_DELETE;
+    operation.request = request;
 
-    cache_->Delete(
-        CopyFetchRequest(request),
-        base::Bind(&CacheStorageCacheTest::ErrorTypeCallback,
-                   base::Unretained(this), base::Unretained(loop.get())));
-    loop->Run();
-
-    return callback_error_ == CacheStorageCache::ERROR_TYPE_OK;
+    CacheStorageCache::ErrorType error =
+        BatchOperation(std::vector<CacheStorageBatchOperation>(1, operation));
+    return error == CacheStorageCache::ERROR_TYPE_OK;
   }
 
   bool Keys() {
@@ -459,6 +460,39 @@ TEST_P(CacheStorageCacheTestP, PutBody) {
   EXPECT_TRUE(Put(body_request_, body_response_));
 }
 
+TEST_P(CacheStorageCacheTestP, PutBody_Multiple) {
+  CacheStorageBatchOperation operation1;
+  operation1.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation1.request = body_request_;
+  operation1.request.url = GURL("http://example.com/1");
+  operation1.response = body_response_;
+  operation1.response.url = GURL("http://example.com/1");
+
+  CacheStorageBatchOperation operation2;
+  operation2.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation2.request = body_request_;
+  operation2.request.url = GURL("http://example.com/2");
+  operation2.response = body_response_;
+  operation2.response.url = GURL("http://example.com/2");
+
+  CacheStorageBatchOperation operation3;
+  operation3.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation3.request = body_request_;
+  operation3.request.url = GURL("http://example.com/3");
+  operation3.response = body_response_;
+  operation3.response.url = GURL("http://example.com/3");
+
+  std::vector<CacheStorageBatchOperation> operations;
+  operations.push_back(operation1);
+  operations.push_back(operation2);
+  operations.push_back(operation3);
+
+  EXPECT_EQ(CacheStorageCache::ERROR_TYPE_OK, BatchOperation(operations));
+  EXPECT_TRUE(Match(operation1.request));
+  EXPECT_TRUE(Match(operation2.request));
+  EXPECT_TRUE(Match(operation3.request));
+}
+
 TEST_P(CacheStorageCacheTestP, ResponseURLDiffersFromRequestURL) {
   no_body_response_.url = GURL("http://example.com/foobar");
   EXPECT_STRNE("http://example.com/foobar",
@@ -478,11 +512,16 @@ TEST_P(CacheStorageCacheTestP, ResponseURLEmpty) {
 }
 
 TEST_F(CacheStorageCacheTest, PutBodyDropBlobRef) {
+  CacheStorageBatchOperation operation;
+  operation.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation.request = body_request_;
+  operation.response = body_response_;
+
   scoped_ptr<base::RunLoop> loop(new base::RunLoop());
-  cache_->Put(CopyFetchRequest(body_request_),
-              CopyFetchResponse(body_response_),
-              base::Bind(&CacheStorageCacheTestP::ErrorTypeCallback,
-                         base::Unretained(this), base::Unretained(loop.get())));
+  cache_->BatchOperation(
+      std::vector<CacheStorageBatchOperation>(1, operation),
+      base::Bind(&CacheStorageCacheTestP::ErrorTypeCallback,
+                 base::Unretained(this), base::Unretained(loop.get())));
   // The handle should be held by the cache now so the deref here should be
   // okay.
   blob_handle_.reset();
@@ -505,6 +544,28 @@ TEST_P(CacheStorageCacheTestP, PutReplace) {
   EXPECT_FALSE(callback_response_data_);
 }
 
+TEST_P(CacheStorageCacheTestP, PutReplcaceInBatch) {
+  CacheStorageBatchOperation operation1;
+  operation1.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation1.request = body_request_;
+  operation1.response = no_body_response_;
+
+  CacheStorageBatchOperation operation2;
+  operation2.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation2.request = body_request_;
+  operation2.response = body_response_;
+
+  std::vector<CacheStorageBatchOperation> operations;
+  operations.push_back(operation1);
+  operations.push_back(operation2);
+
+  EXPECT_EQ(CacheStorageCache::ERROR_TYPE_OK, BatchOperation(operations));
+
+  // |operation2| should win.
+  EXPECT_TRUE(Match(operation2.request));
+  EXPECT_TRUE(callback_response_data_);
+}
+

[jkarlin, 2015/05/08 11:45:43]

Add a test to show that you can mix delete and put operations in the same batch
call.

[jkarlin, 2015/05/08 11:45:43]

Also add a test that if the first batch operation (of several) fails it still
returns an error. 

[nhiroki, 2015/05/08 15:09:01]

Do you have any ideas to fail only one operation in the batch? I tried to add
such a test using an invalid request/response whose protocol is not supported
(eg. "ftp://") but the current backend implementation does not check the
protocol and successfully completes the operation...

(If there isn't a reasonable way, is it acceptable to revisit this after adding
those security checks in the backend?)

[nhiroki, 2015/05/08 15:09:01]

The current implementation has "DCHECK_EQ(1u, operations.size())" in
CacheStorageCache::BatchOperation() to avoid such mixed operations.

I'm not really sure that we need to support mixed operations because the spec
does not define the case. I suppose that mixed operations make it difficult to
handle batch results because a delete operation can return NOT_FOUND as a
success case.

[jkarlin, 2015/05/08 15:27:27]

Yes, please add a TODO to add the test and add a bug to add the security checks
on the browser side.

[jkarlin, 2015/05/08 15:27:27]

Good point. Never mind on this test.

[nhiroki, 2015/05/11 07:35:04]

Done.

 TEST_P(CacheStorageCacheTestP, MatchNoBody) {
   EXPECT_TRUE(Put(no_body_request_, no_body_response_));
   EXPECT_TRUE(Match(no_body_request_));
@@ -764,23 +825,29 @@ TEST_P(CacheStorageCacheTestP, VerifySerialScheduling) {
 
   int sequence_out = -1;
 
-  scoped_ptr<ServiceWorkerResponse> response1 =
-      CopyFetchResponse(body_response_);
+  CacheStorageBatchOperation operation1;
+  operation1.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation1.request = body_request_;
+  operation1.response = body_response_;
+
   scoped_ptr<base::RunLoop> close_loop1(new base::RunLoop());
-  cache_->Put(
-      CopyFetchRequest(body_request_), response1.Pass(),
+  cache_->BatchOperation(
+      std::vector<CacheStorageBatchOperation>(1, operation1),
       base::Bind(&CacheStorageCacheTest::SequenceCallback,
                  base::Unretained(this), 1, &sequence_out, close_loop1.get()));
 
   // Blocks on opening the cache entry.
   base::RunLoop().RunUntilIdle();
 
+  CacheStorageBatchOperation operation2;
+  operation2.operation_type = CACHE_STORAGE_CACHE_OPERATION_TYPE_PUT;
+  operation2.request = body_request_;
+  operation2.response = body_response_;
+
   delayable_backend->set_delay_open(false);
-  scoped_ptr<ServiceWorkerResponse> response2 =
-      CopyFetchResponse(body_response_);
   scoped_ptr<base::RunLoop> close_loop2(new base::RunLoop());
-  cache_->Put(
-      CopyFetchRequest(body_request_), response2.Pass(),
+  cache_->BatchOperation(
+      std::vector<CacheStorageBatchOperation>(1, operation2),
       base::Bind(&CacheStorageCacheTest::SequenceCallback,
                  base::Unretained(this), 2, &sequence_out, close_loop2.get()));