CacheStorage: Support multiple batch operations

content/browser/cache_storage/cache_storage_cache_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/cache_storage/cache_storage_cache.h"
 
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/run_loop.h"
@@ skipping 234 matching lines @@
         blink::WebServiceWorkerResponseTypeDefault, headers, "", 0, GURL());
   }
 
   scoped_ptr<ServiceWorkerFetchRequest> CopyFetchRequest(
       const ServiceWorkerFetchRequest& request) {
     return make_scoped_ptr(new ServiceWorkerFetchRequest(
         request.url, request.method, request.headers, request.referrer,
         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()
     // once the cache uses a passed in MessageLoopProxy instead of the CACHE
     // 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) {
     scoped_ptr<base::RunLoop> loop(new base::RunLoop());
 
     cache_->Match(
         CopyFetchRequest(request),
         base::Bind(&CacheStorageCacheTest::ResponseAndErrorCallback,
                    base::Unretained(this), base::Unretained(loop.get())));
     loop->Run();
 
     return callback_error_ == CacheStorageCache::ERROR_TYPE_OK;
   }
 
   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() {
     scoped_ptr<base::RunLoop> loop(new base::RunLoop());
 
     cache_->Keys(base::Bind(&CacheStorageCacheTest::RequestsCallback,
                             base::Unretained(this),
                             base::Unretained(loop.get())));
     loop->Run();
 
@@ skipping 140 matching lines @@
 };
 
 TEST_P(CacheStorageCacheTestP, PutNoBody) {
   EXPECT_TRUE(Put(no_body_request_, no_body_response_));
 }
 
 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",
                no_body_request_.url.spec().c_str());
   EXPECT_TRUE(Put(no_body_request_, no_body_response_));
   EXPECT_TRUE(Match(no_body_request_));
   EXPECT_STREQ("http://example.com/foobar",
                callback_response_->url.spec().c_str());
 }
 
 TEST_P(CacheStorageCacheTestP, ResponseURLEmpty) {
   no_body_response_.url = GURL();
   EXPECT_STRNE("", no_body_request_.url.spec().c_str());
   EXPECT_TRUE(Put(no_body_request_, no_body_response_));
   EXPECT_TRUE(Match(no_body_request_));
   EXPECT_STREQ("", callback_response_->url.spec().c_str());
 }
 
 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();
   loop->Run();
 
   EXPECT_EQ(CacheStorageCache::ERROR_TYPE_OK, callback_error_);
 }
 
 TEST_P(CacheStorageCacheTestP, PutReplace) {
   EXPECT_TRUE(Put(body_request_, no_body_response_));
   EXPECT_TRUE(Match(body_request_));
   EXPECT_FALSE(callback_response_data_);
 
   EXPECT_TRUE(Put(body_request_, body_response_));
   EXPECT_TRUE(Match(body_request_));
   EXPECT_TRUE(callback_response_data_);
 
   EXPECT_TRUE(Put(body_request_, no_body_response_));
   EXPECT_TRUE(Match(body_request_));
   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_));
   EXPECT_EQ(200, callback_response_->status_code);
   EXPECT_STREQ("OK", callback_response_->status_text.c_str());
   EXPECT_STREQ("http://example.com/no_body.html",
                callback_response_->url.spec().c_str());
   EXPECT_STREQ("", callback_response_->blob_uuid.c_str());
   EXPECT_EQ(0u, callback_response_->blob_size);
 }
@@ skipping 239 matching lines @@
 
 TEST_P(CacheStorageCacheTestP, VerifySerialScheduling) {
   // Start two operations, the first one is delayed but the second isn't. The
   // second should wait for the first.
   EXPECT_TRUE(Keys());  // Opens the backend.
   DelayableBackend* delayable_backend = cache_->UseDelayableBackend();
   delayable_backend->set_delay_open(true);
 
   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()));
 
   // The second put operation should wait for the first to complete.
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(callback_response_);
 
   delayable_backend->OpenEntryContinue();
   close_loop1->Run();
   EXPECT_EQ(1, sequence_out);
   close_loop2->Run();
   EXPECT_EQ(2, sequence_out);
 }
 
 INSTANTIATE_TEST_CASE_P(CacheStorageCacheTest,
                         CacheStorageCacheTestP,
                         ::testing::Values(false, true));
 
 }  // namespace content