service worker: Reduce direct NewResourceId() calls in tests

* Let WriteToDiskCache{A,S}sync() assign resource ID internally
* Add WriteToDiskCacheWithIdSync() that works as the same as the
  previous WriteToDiskCacheSync()

NewResourceId() call will become async once we move ServiceWorkerStorage
to the storage service. This CL reduces NewResourceId() calls so that
we don't have to make all call sites async in the future.

Bug: 1046335
Change-Id: I9ad57f8920bd5e157bd4ba8262699f72148b5bac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2035637
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738768}

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -177,9 +177,8 @@ void EmbeddedWorkerTestHelper::PopulateScriptCacheMap(
     // Add a dummy ResourceRecord for the main script to the script cache map of
     // the ServiceWorkerVersion.
     WriteToDiskCacheAsync(
-        context()->storage(), version->script_url(),
-        context()->storage()->NewResourceId(), {} /* headers */, "I'm a body",
-        "I'm a meta data",
+        context()->storage(), version->script_url(), {} /* headers */,
+        "I'm a body", "I'm a meta data",
         base::BindOnce(
             [](scoped_refptr<ServiceWorkerVersion> version,
                base::OnceClosure callback,

content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc

@@ -109,9 +109,8 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
 
     std::vector<ServiceWorkerDatabase::ResourceRecord> records;
     records.push_back(WriteToDiskCacheSync(
-        context()->storage(), version_->script_url(),
-        context()->storage()->NewResourceId(), {} /* headers */, "I'm a body",
-        "I'm a meta data"));
+        context()->storage(), version_->script_url(), {} /* headers */,
+        "I'm a body", "I'm a meta data"));
     version_->script_cache_map()->SetResources(records);
     version_->SetMainScriptHttpResponseInfo(
         EmbeddedWorkerTestHelper::CreateHttpResponseInfo());

content/browser/service_worker/service_worker_installed_scripts_sender_unittest.cc

@@ -93,8 +93,8 @@ class ExpectedScriptInfo {
 
   ServiceWorkerDatabase::ResourceRecord WriteToDiskCache(
       ServiceWorkerStorage* storage) const {
-    return ::content::WriteToDiskCacheSync(storage, script_url_, resource_id_,
-                                           headers_, body_, meta_data_);
+    return ::content::WriteToDiskCacheWithIdSync(
+        storage, script_url_, resource_id_, headers_, body_, meta_data_);
   }
 
   void CheckIfIdentical(

content/browser/service_worker/service_worker_navigation_loader_unittest.cc

@@ -347,9 +347,9 @@ class ServiceWorkerNavigationLoaderTest : public testing::Test {
         registration_.get(), GURL("https://example.com/service_worker.js"),
         blink::mojom::ScriptType::kClassic);
     std::vector<ServiceWorkerDatabase::ResourceRecord> records;
-    records.push_back(WriteToDiskCacheSync(
-        storage(), version_->script_url(), storage()->NewResourceId(),
-        {} /* headers */, "I'm the body", "I'm the meta data"));
+    records.push_back(WriteToDiskCacheSync(storage(), version_->script_url(),
+                                           {} /* headers */, "I'm the body",
+                                           "I'm the meta data"));
     version_->script_cache_map()->SetResources(records);
     version_->set_fetch_handler_existence(
         ServiceWorkerVersion::FetchHandlerExistence::EXISTS);

content/browser/service_worker/service_worker_registration_unittest.cc

@@ -406,8 +406,7 @@ class ServiceWorkerActivationTest : public ServiceWorkerRegistrationTest,
     std::vector<ServiceWorkerDatabase::ResourceRecord> records_1;
     records_1.push_back(WriteToDiskCacheSync(
         helper_->context()->storage(), version_1->script_url(),
-        helper_->context()->storage()->NewResourceId(), {} /* headers */,
-        "I'm the body", "I'm the meta data"));
+        {} /* headers */, "I'm the body", "I'm the meta data"));
     version_1->script_cache_map()->SetResources(records_1);
     version_1->SetMainScriptHttpResponseInfo(
         EmbeddedWorkerTestHelper::CreateHttpResponseInfo());
@@ -448,8 +447,7 @@ class ServiceWorkerActivationTest : public ServiceWorkerRegistrationTest,
     std::vector<ServiceWorkerDatabase::ResourceRecord> records_2;
     records_2.push_back(WriteToDiskCacheSync(
         helper_->context()->storage(), version_2->script_url(),
-        helper_->context()->storage()->NewResourceId(), {} /* headers */,
-        "I'm the body", "I'm the meta data"));
+        {} /* headers */, "I'm the body", "I'm the meta data"));
     version_2->script_cache_map()->SetResources(records_2);
     version_2->SetMainScriptHttpResponseInfo(
         EmbeddedWorkerTestHelper::CreateHttpResponseInfo());
@@ -892,9 +890,9 @@ class ServiceWorkerRegistrationObjectHostTest
         context()->registry()->CreateNewVersion(
             registration, script_url, blink::mojom::ScriptType::kClassic);
     std::vector<ServiceWorkerDatabase::ResourceRecord> records;
-    records.push_back(WriteToDiskCacheSync(
-        storage(), version->script_url(), storage()->NewResourceId(),
-        {} /* headers */, "I'm the body", "I'm the meta data"));
+    records.push_back(WriteToDiskCacheSync(storage(), version->script_url(),
+                                           {} /* headers */, "I'm the body",
+                                           "I'm the meta data"));
     version->script_cache_map()->SetResources(records);
     version->SetMainScriptHttpResponseInfo(
         EmbeddedWorkerTestHelper::CreateHttpResponseInfo());

content/browser/service_worker/service_worker_script_loader_factory_unittest.cc

@@ -132,8 +132,9 @@ class ServiceWorkerScriptLoaderFactoryCopyResumeTest
 
   void SetUp() override {
     ServiceWorkerScriptLoaderFactoryTest::SetUp();
-    WriteToDiskCacheSync(helper_->context()->storage(), script_url_,
-                         kOldResourceId, kOldHeaders, kOldData, std::string());
+    WriteToDiskCacheWithIdSync(helper_->context()->storage(), script_url_,
+                               kOldResourceId, kOldHeaders, kOldData,
+                               std::string());
   }
 
   void CheckResponse(const std::string& expected_body) {

content/browser/service_worker/service_worker_test_utils.cc

@@ -195,6 +195,57 @@ void OnWriteBodyInfoToDiskCache(
                      meta_data, std::move(callback)));
 }
 
+void WriteToDiskCacheAsyncInternal(
+    const GURL& script_url,
+    const std::vector<std::pair<std::string, std::string>>& headers,
+    const std::string& body,
+    const std::string& meta_data,
+    std::unique_ptr<ServiceWorkerResponseWriter> body_writer,
+    std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer,
+    WriteToDiskCacheCallback callback) {
+  std::unique_ptr<net::HttpResponseInfo> http_info =
+      std::make_unique<net::HttpResponseInfo>();
+  http_info->request_time = base::Time::Now();
+  http_info->response_time = base::Time::Now();
+  http_info->headers =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.0 200 OK\0\0");
+  for (const auto& header : headers)
+    http_info->headers->AddHeader(header.first + ": " + header.second);
+
+  scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =
+      base::MakeRefCounted<HttpResponseInfoIOBuffer>(std::move(http_info));
+  info_buffer->response_data_size = body.size();
+  ServiceWorkerResponseWriter* writer_rawptr = body_writer.get();
+  writer_rawptr->WriteInfo(
+      info_buffer.get(),
+      base::BindOnce(&OnWriteBodyInfoToDiskCache, std::move(body_writer),
+                     std::move(metadata_writer), script_url, body, meta_data,
+                     std::move(callback)));
+}
+
+ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSyncInternal(
+    const GURL& script_url,
+    const std::vector<std::pair<std::string, std::string>>& headers,
+    const std::string& body,
+    const std::string& meta_data,
+    std::unique_ptr<ServiceWorkerResponseWriter> body_writer,
+    std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer) {
+  ServiceWorkerDatabase::ResourceRecord record;
+
+  base::RunLoop loop;
+  WriteToDiskCacheAsyncInternal(
+      script_url, headers, body, meta_data, std::move(body_writer),
+      std::move(metadata_writer),
+      base::BindLambdaForTesting(
+          [&](ServiceWorkerDatabase::ResourceRecord result) {
+            record = result;
+            loop.Quit();
+          }));
+  loop.Run();
+
+  return record;
+}
+
 }  // namespace
 
 ServiceWorkerRemoteProviderEndpoint::ServiceWorkerRemoteProviderEndpoint() {}
@@ -367,60 +418,51 @@ CreateServiceWorkerRegistrationAndVersion(ServiceWorkerContextCore* context,
   return registration;
 }
 
-ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
+ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheWithIdSync(
     ServiceWorkerStorage* storage,
     const GURL& script_url,
     int64_t resource_id,
     const std::vector<std::pair<std::string, std::string>>& headers,
     const std::string& body,
     const std::string& meta_data) {
-  ServiceWorkerDatabase::ResourceRecord record;
-
-  base::RunLoop loop;
-  WriteToDiskCacheAsync(
-      storage, script_url, resource_id, headers, body, meta_data,
-      base::BindOnce(
-          [](ServiceWorkerDatabase::ResourceRecord* out_record,
-             base::OnceClosure quit_closure,
-             ServiceWorkerDatabase::ResourceRecord result) {
-            *out_record = result;
-            std::move(quit_closure).Run();
-          },
-          &record, loop.QuitClosure()));
-  loop.Run();
+  std::unique_ptr<ServiceWorkerResponseWriter> body_writer =
+      storage->CreateResponseWriter(resource_id);
+  std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer =
+      storage->CreateResponseMetadataWriter(resource_id);
+  return WriteToDiskCacheSyncInternal(script_url, headers, body, meta_data,
+                                      std::move(body_writer),
+                                      std::move(metadata_writer));
+}
 
-  return record;
+ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
+    ServiceWorkerStorage* storage,
+    const GURL& script_url,
+    const std::vector<std::pair<std::string, std::string>>& headers,
+    const std::string& body,
+    const std::string& meta_data) {
+  std::unique_ptr<ServiceWorkerResponseWriter> body_writer =
+      CreateNewResponseWriterSync(storage);
+  std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer =
+      storage->CreateResponseMetadataWriter(body_writer->response_id());
+  return WriteToDiskCacheSyncInternal(script_url, headers, body, meta_data,
+                                      std::move(body_writer),
+                                      std::move(metadata_writer));
 }
 
 void WriteToDiskCacheAsync(
     ServiceWorkerStorage* storage,
     const GURL& script_url,
-    int64_t resource_id,
     const std::vector<std::pair<std::string, std::string>>& headers,
     const std::string& body,
     const std::string& meta_data,
     WriteToDiskCacheCallback callback) {
-  std::unique_ptr<net::HttpResponseInfo> http_info =
-      std::make_unique<net::HttpResponseInfo>();
-  http_info->request_time = base::Time::Now();
-  http_info->response_time = base::Time::Now();
-  http_info->headers =
-      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.0 200 OK\0\0");
-  for (const auto& header : headers)
-    http_info->headers->AddHeader(header.first + ": " + header.second);
-
-  auto body_writer = storage->CreateResponseWriter(resource_id);
-  auto metadata_writer = storage->CreateResponseMetadataWriter(resource_id);
-
-  scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =
-      base::MakeRefCounted<HttpResponseInfoIOBuffer>(std::move(http_info));
-  info_buffer->response_data_size = body.size();
-  ServiceWorkerResponseWriter* writer_rawptr = body_writer.get();
-  writer_rawptr->WriteInfo(
-      info_buffer.get(),
-      base::BindOnce(&OnWriteBodyInfoToDiskCache, std::move(body_writer),
-                     std::move(metadata_writer), script_url, body, meta_data,
-                     std::move(callback)));
+  std::unique_ptr<ServiceWorkerResponseWriter> body_writer =
+      CreateNewResponseWriterSync(storage);
+  std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer =
+      storage->CreateResponseMetadataWriter(body_writer->response_id());
+  WriteToDiskCacheAsyncInternal(
+      script_url, headers, body, meta_data, std::move(body_writer),
+      std::move(metadata_writer), std::move(callback));
 }
 
 std::unique_ptr<ServiceWorkerResponseWriter> CreateNewResponseWriterSync(

content/browser/service_worker/service_worker_test_utils.h

@@ -154,7 +154,7 @@ CreateServiceWorkerRegistrationAndVersion(ServiceWorkerContextCore* context,
 // base::RunLoop since base::RunLoop is used internally to wait for completing
 // all of tasks. If it's in another base::RunLoop, consider to use
 // WriteToDiskCacheAsync().
-ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
+ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheWithIdSync(
     ServiceWorkerStorage* storage,
     const GURL& script_url,
     int64_t resource_id,
@@ -162,6 +162,15 @@ ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
     const std::string& body,
     const std::string& meta_data);
 
+// Similar to WriteToDiskCacheWithIdSync() but instead of taking a resource id,
+// this assigns a new resource ID internally.
+ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
+    ServiceWorkerStorage* storage,
+    const GURL& script_url,
+    const std::vector<std::pair<std::string, std::string>>& headers,
+    const std::string& body,
+    const std::string& meta_data);
+
 using WriteToDiskCacheCallback =
     base::OnceCallback<void(ServiceWorkerDatabase::ResourceRecord record)>;
 
@@ -172,7 +181,6 @@ using WriteToDiskCacheCallback =
 void WriteToDiskCacheAsync(
     ServiceWorkerStorage* storage,
     const GURL& script_url,
-    int64_t resource_id,
     const std::vector<std::pair<std::string, std::string>>& headers,
     const std::string& body,
     const std::string& meta_data,

content/browser/service_worker/service_worker_updated_script_loader_unittest.cc

@@ -84,8 +84,8 @@ class ServiceWorkerUpdatedScriptLoaderTest : public testing::Test {
     SetUpRegistration(kScriptURL);
 
     // Create the old script resource in storage.
-    WriteToDiskCacheSync(context()->storage(), kScriptURL, kOldResourceId,
-                         kOldHeaders, kOldData, std::string());
+    WriteToDiskCacheWithIdSync(context()->storage(), kScriptURL, kOldResourceId,
+                               kOldHeaders, kOldData, std::string());
   }
 
   // Sets up ServiceWorkerRegistration and ServiceWorkerVersion. This should be

content/browser/service_worker/service_worker_version_unittest.cc

@@ -100,7 +100,7 @@ class ServiceWorkerVersionTest : public testing::Test {
         blink::mojom::ScriptType::kClassic);
     EXPECT_EQ(url::Origin::Create(scope_), version_->script_origin());
     std::vector<ServiceWorkerDatabase::ResourceRecord> records;
-    records.push_back(WriteToDiskCacheSync(
+    records.push_back(WriteToDiskCacheWithIdSync(
         helper_->context()->storage(), version_->script_url(), 10,
         {} /* headers */, "I'm a body", "I'm a meta data"));
     version_->script_cache_map()->SetResources(records);