service worker: Rewrite WriteToDiskCacheAsync()

Before this CL, WriteToDiskCacheAsync() synchronously returned
ServiceWorkerDatabase::ResourceRecord even its internal operations
were asynchornously done. This behavior is undesirable because
we want to make resource ID assignment async but creating
ResourceRecord requires a new ID.

This CL make WriteToDiskCacheAsync() return ResourceRecord
asynchornously. A subsequent CL will remove |resource_id| argument
from the function and assign an ID inside the function.

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

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -165,23 +165,34 @@ net::HttpResponseInfo EmbeddedWorkerTestHelper::CreateHttpResponseInfo() {
 void EmbeddedWorkerTestHelper::PopulateScriptCacheMap(
     int64_t version_id,
     base::OnceClosure callback) {
-  ServiceWorkerVersion* version = context()->GetLiveVersion(version_id);
+  scoped_refptr<ServiceWorkerVersion> version =
+      context()->GetLiveVersion(version_id);
   if (!version) {
     std::move(callback).Run();
     return;
   }
+  if (!version->GetMainScriptHttpResponseInfo())
+    version->SetMainScriptHttpResponseInfo(CreateHttpResponseInfo());
   if (!version->script_cache_map()->size()) {
-    std::vector<ServiceWorkerDatabase::ResourceRecord> records;
     // Add a dummy ResourceRecord for the main script to the script cache map of
     // the ServiceWorkerVersion.
-    records.push_back(WriteToDiskCacheAsync(
+    WriteToDiskCacheAsync(
         context()->storage(), version->script_url(),
         context()->storage()->NewResourceId(), {} /* headers */, "I'm a body",
-        "I'm a meta data", std::move(callback)));
-    version->script_cache_map()->SetResources(records);
+        "I'm a meta data",
+        base::BindOnce(
+            [](scoped_refptr<ServiceWorkerVersion> version,
+               base::OnceClosure callback,
+               ServiceWorkerDatabase::ResourceRecord record) {
+              std::vector<ServiceWorkerDatabase::ResourceRecord> records;
+              records.push_back(std::move(record));
+              version->script_cache_map()->SetResources(records);
+
+              std::move(callback).Run();
+            },
+            version, std::move(callback)));
+    return;
   }
-  if (!version->GetMainScriptHttpResponseInfo())
-    version->SetMainScriptHttpResponseInfo(CreateHttpResponseInfo());
   // Call |callback| if |version| already has ResourceRecords.
   if (!callback.is_null())
     std::move(callback).Run();

content/browser/service_worker/service_worker_test_utils.cc

@@ -144,81 +144,55 @@ class FakeNavigationClient : public mojom::NavigationClient {
   DISALLOW_COPY_AND_ASSIGN(FakeNavigationClient);
 };
 
-void OnWriteBodyInfoToDiskCache(
-    std::unique_ptr<ServiceWorkerResponseWriter> writer,
-    const std::string& body,
-    base::OnceClosure callback,
+void OnWriteMetadataToDiskCache(
+    std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer,
+    const GURL& script_url,
+    int body_size,
+    int meta_data_size,
+    WriteToDiskCacheCallback callback,
     int result) {
-  EXPECT_GE(result, 0);
-  scoped_refptr<net::IOBuffer> body_buffer =
-      base::MakeRefCounted<net::StringIOBuffer>(body);
-  ServiceWorkerResponseWriter* writer_rawptr = writer.get();
-  writer_rawptr->WriteData(
-      body_buffer.get(), body.size(),
-      base::BindOnce(
-          [](std::unique_ptr<ServiceWorkerResponseWriter> /* unused */,
-             base::OnceClosure callback, int expected, int result) {
-            EXPECT_EQ(expected, result);
-            std::move(callback).Run();
-          },
-          std::move(writer), std::move(callback), body.size()));
-}
-
-void WriteBodyToDiskCache(std::unique_ptr<ServiceWorkerResponseWriter> writer,
-                          std::unique_ptr<net::HttpResponseInfo> info,
-                          const std::string& body,
-                          base::OnceClosure callback) {
-  scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =
-      base::MakeRefCounted<HttpResponseInfoIOBuffer>(std::move(info));
-  info_buffer->response_data_size = body.size();
-  ServiceWorkerResponseWriter* writer_rawptr = writer.get();
-  writer_rawptr->WriteInfo(
-      info_buffer.get(),
-      base::BindOnce(&OnWriteBodyInfoToDiskCache, std::move(writer), body,
-                     std::move(callback)));
+  EXPECT_EQ(result, meta_data_size);
+  std::move(callback).Run(ServiceWorkerDatabase::ResourceRecord(
+      metadata_writer->response_id(), script_url, body_size));
 }
 
-void WriteMetaDataToDiskCache(
-    std::unique_ptr<ServiceWorkerResponseMetadataWriter> writer,
+void OnWriteBodyDataToDiskCache(
+    std::unique_ptr<ServiceWorkerResponseWriter> writer,
+    std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer,
+    const GURL& script_url,
+    int body_size,
     const std::string& meta_data,
-    base::OnceClosure callback) {
+    WriteToDiskCacheCallback callback,
+    int result) {
+  EXPECT_EQ(result, body_size);
   scoped_refptr<net::IOBuffer> meta_data_buffer =
       base::MakeRefCounted<net::StringIOBuffer>(meta_data);
-  ServiceWorkerResponseMetadataWriter* writer_rawptr = writer.get();
-  writer_rawptr->WriteMetadata(
+  ServiceWorkerResponseMetadataWriter* metadata_writer_rawptr =
+      metadata_writer.get();
+  metadata_writer_rawptr->WriteMetadata(
       meta_data_buffer.get(), meta_data.size(),
-      base::BindOnce(
-          [](std::unique_ptr<ServiceWorkerResponseMetadataWriter> /* unused */,
-             base::OnceClosure callback, int expected, int result) {
-            EXPECT_EQ(expected, result);
-            std::move(callback).Run();
-          },
-          std::move(writer), std::move(callback), meta_data.size()));
+      base::BindOnce(&OnWriteMetadataToDiskCache, std::move(metadata_writer),
+                     script_url, body_size, meta_data.size(),
+                     std::move(callback)));
 }
 
-// Writes the script with custom net::HttpResponseInfo down to |storage|
-// asynchronously. When completing tasks, |callback| will be called. You must
-// wait for |callback| instead of base::RunUntilIdle because wiriting to the
-// storage might happen on another thread and base::RunLoop could get idle
-// before writes has not finished yet.
-ServiceWorkerDatabase::ResourceRecord
-WriteToDiskCacheWithCustomResponseInfoAsync(
-    ServiceWorkerStorage* storage,
+void OnWriteBodyInfoToDiskCache(
+    std::unique_ptr<ServiceWorkerResponseWriter> writer,
+    std::unique_ptr<ServiceWorkerResponseMetadataWriter> metadata_writer,
     const GURL& script_url,
-    int64_t resource_id,
-    std::unique_ptr<net::HttpResponseInfo> http_info,
     const std::string& body,
     const std::string& meta_data,
-    base::OnceClosure callback) {
-  base::RepeatingClosure barrier = base::BarrierClosure(2, std::move(callback));
-  auto body_writer = storage->CreateResponseWriter(resource_id);
-  WriteBodyToDiskCache(std::move(body_writer), std::move(http_info), body,
-                       barrier);
-  auto metadata_writer = storage->CreateResponseMetadataWriter(resource_id);
-  WriteMetaDataToDiskCache(std::move(metadata_writer), meta_data,
-                           std::move(barrier));
-  return ServiceWorkerDatabase::ResourceRecord(resource_id, script_url,
-                                               body.size());
+    WriteToDiskCacheCallback callback,
+    int result) {
+  EXPECT_GE(result, 0);
+  scoped_refptr<net::IOBuffer> body_buffer =
+      base::MakeRefCounted<net::StringIOBuffer>(body);
+  ServiceWorkerResponseWriter* writer_rawptr = writer.get();
+  writer_rawptr->WriteData(
+      body_buffer.get(), body.size(),
+      base::BindOnce(&OnWriteBodyDataToDiskCache, std::move(writer),
+                     std::move(metadata_writer), script_url, body.size(),
+                     meta_data, std::move(callback)));
 }
 
 }  // namespace
@@ -400,33 +374,53 @@ ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
     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;
-  ServiceWorkerDatabase::ResourceRecord record =
-      WriteToDiskCacheAsync(storage, script_url, resource_id, headers, body,
-                            meta_data, loop.QuitClosure());
+  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();
+
   return record;
 }
 
-ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheAsync(
+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,
-    base::OnceClosure callback) {
-  std::unique_ptr<net::HttpResponseInfo> info =
+    WriteToDiskCacheCallback callback) {
+  std::unique_ptr<net::HttpResponseInfo> http_info =
       std::make_unique<net::HttpResponseInfo>();
-  info->request_time = base::Time::Now();
-  info->response_time = base::Time::Now();
-  info->headers =
+  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)
-    info->headers->AddHeader(header.first + ": " + header.second);
-  return WriteToDiskCacheWithCustomResponseInfoAsync(
-      storage, script_url, resource_id, std::move(info), body, meta_data,
-      std::move(callback));
+    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> CreateNewResponseWriterSync(

content/browser/service_worker/service_worker_test_utils.h

@@ -162,18 +162,21 @@ ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheSync(
     const std::string& body,
     const std::string& meta_data);
 
+using WriteToDiskCacheCallback =
+    base::OnceCallback<void(ServiceWorkerDatabase::ResourceRecord record)>;
+
 // Writes the script down to |storage| asynchronously. When completing tasks,
 // |callback| will be called. You must wait for |callback| instead of
 // base::RunUntilIdle because wiriting to the storage might happen on another
 // thread and base::RunLoop could get idle before writes has not finished yet.
-ServiceWorkerDatabase::ResourceRecord WriteToDiskCacheAsync(
+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,
-    base::OnceClosure callback);
+    WriteToDiskCacheCallback callback);
 
 // Calls ServiceWorkerStorage::CreateNewResponseWriter() and returns the
 // created writer synchronously.