Stop using AppCacheResponseMetadataWriter from service workers

This CL removes AppCacheResponseMetadataWriter dependency from
ServiceWorkerResourceMetadataWriter by re-implementing the writer.
See [1] for the reasoning of the rewrite.

This CL should not have behavior changes, and existing tests cover
this change.

[1] https://docs.google.com/document/d/1UhdZnaeEP8GywZR0Gik6wF6jKmNY3je6aDhPfJ05YhU/edit?usp=sharing

Bug: 1117369
Change-Id: I47ce15de539eb23746f1154377f3d8650d951b7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440321
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815776}

content/browser/service_worker/service_worker_disk_cache.cc

@@ -14,9 +14,4 @@ namespace content {
 ServiceWorkerDiskCache::ServiceWorkerDiskCache()
     : AppCacheDiskCache(/*use_simple_cache=*/true) {}
 
-ServiceWorkerResponseMetadataWriter::ServiceWorkerResponseMetadataWriter(
-    int64_t resource_id,
-    base::WeakPtr<AppCacheDiskCache> disk_cache)
-    : AppCacheResponseMetadataWriter(resource_id, std::move(disk_cache)) {}
-
 }  // namespace content

content/browser/service_worker/service_worker_disk_cache.h

@@ -5,13 +5,8 @@
 #ifndef CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_DISK_CACHE_H_
 #define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_DISK_CACHE_H_
 
-#include <stdint.h>
-
-#include "base/memory/weak_ptr.h"
 #include "content/browser/appcache/appcache_disk_cache.h"
-#include "content/browser/appcache/appcache_disk_cache_ops.h"
 #include "content/common/content_export.h"
-#include "services/network/public/mojom/url_response_head.mojom.h"
 
 namespace content {
 
@@ -26,19 +21,6 @@ class CONTENT_EXPORT ServiceWorkerDiskCache : public AppCacheDiskCache {
   ServiceWorkerDiskCache();
 };
 
-// TODO(crbug.com/1117369): Migrate to
-// storage::mojom::ServiceWorkerResourceMetadataWriter.
-class CONTENT_EXPORT ServiceWorkerResponseMetadataWriter
-    : public AppCacheResponseMetadataWriter {
- protected:
-  // Should only be constructed by the storage class.
-  friend class ServiceWorkerStorage;
-
-  ServiceWorkerResponseMetadataWriter(
-      int64_t resource_id,
-      base::WeakPtr<AppCacheDiskCache> disk_cache);
-};
-
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_DISK_CACHE_H_

content/browser/service_worker/service_worker_resource_ops.h

@@ -13,16 +13,15 @@ namespace content {
 
 class BigIOBuffer;
 
-// Manages a service worker disk cache entry. Creates and owns an entry.
-// TODO(bashi): Used by resource writers. Use in readers as well.
-class DiskEntryManager {
+// Creates and owns a service worker disk cacke entry.
+class DiskEntryCreator {
  public:
-  DiskEntryManager(int64_t resource_id,
+  DiskEntryCreator(int64_t resource_id,
                    base::WeakPtr<AppCacheDiskCache> disk_cache);
-  ~DiskEntryManager();
+  ~DiskEntryCreator();
 
-  DiskEntryManager(const DiskEntryManager&) = delete;
-  DiskEntryManager& operator=(const DiskEntryManager&) = delete;
+  DiskEntryCreator(const DiskEntryCreator&) = delete;
+  DiskEntryCreator& operator=(const DiskEntryCreator&) = delete;
 
   // Can be nullptr when a disk cache error occurs.
   AppCacheDiskCacheEntry* entry() {
@@ -58,14 +57,14 @@ class DiskEntryManager {
   // TODO(crbug.com/586174): Refactor service worker's disk cache to use
   // disk_cache::EntryResult to make these callbacks non-static.
   static void DidCreateEntryForFirstAttempt(
-      base::WeakPtr<DiskEntryManager> entry_manager,
+      base::WeakPtr<DiskEntryCreator> entry_creator,
       AppCacheDiskCacheEntry** entry,
       int rv);
   static void DidDoomExistingEntry(
-      base::WeakPtr<DiskEntryManager> entry_manager,
+      base::WeakPtr<DiskEntryCreator> entry_creator,
       int rv);
   static void DidCreateEntryForSecondAttempt(
-      base::WeakPtr<DiskEntryManager> entry_manager,
+      base::WeakPtr<DiskEntryCreator> entry_creator,
       AppCacheDiskCacheEntry** entry,
       int rv);
 
@@ -80,7 +79,44 @@ class DiskEntryManager {
   // Stored as a data member to handle //net-style maybe-async methods.
   base::OnceClosure ensure_entry_is_created_callback_;
 
-  base::WeakPtrFactory<DiskEntryManager> weak_factory_{this};
+  base::WeakPtrFactory<DiskEntryCreator> weak_factory_{this};
+};
+
+// Opens and owns a service worker disk cache entry.
+class DiskEntryOpener {
+ public:
+  DiskEntryOpener(int64_t resource_id,
+                  base::WeakPtr<AppCacheDiskCache> disk_cache);
+  ~DiskEntryOpener();
+
+  DiskEntryOpener(const DiskEntryOpener&) = delete;
+  DiskEntryOpener& operator=(const DiskEntryOpener&) = delete;
+
+  // Can be nullptr when a disk cache error occurs.
+  AppCacheDiskCacheEntry* entry() { return entry_; }
+
+  // Calls the callback when entry() is opened and can be used.
+  //
+  // If necessary, opens a disk cache entry for the `resource_id` passed to the
+  // constructor. After the callback is called, `entry()` can be safely called
+  // to obtain the created entry.
+  void EnsureEntryIsOpen(base::OnceClosure callback);
+
+ private:
+  // TODO(crbug.com/586174): Refactor service worker's disk cache to use
+  // disk_cache::EntryResult to make this callback non-static.
+  static void DidOpenEntry(base::WeakPtr<DiskEntryOpener> entry_creator,
+                           AppCacheDiskCacheEntry** entry,
+                           int rv);
+
+  const int64_t resource_id_;
+  base::WeakPtr<AppCacheDiskCache> disk_cache_;
+  AppCacheDiskCacheEntry* entry_ = nullptr;
+
+  // Stored as a data member to handle //net-style maybe-async methods.
+  base::OnceClosure ensure_entry_is_opened_callback_;
+
+  base::WeakPtrFactory<DiskEntryOpener> weak_factory_{this};
 };
 
 // The implementation of storage::mojom::ServiceWorkerResourceReader.
@@ -120,18 +156,7 @@ class ServiceWorkerResourceReaderImpl
   // data.
   void DidReadDataComplete();
 
-  // Opens a disk cache entry associated with `resource_id_`, if it isn't
-  // opened yet.
-  void EnsureEntryIsOpen(base::OnceClosure callback);
-
-  static void DidOpenEntry(
-      base::WeakPtr<ServiceWorkerResourceReaderImpl> reader,
-      AppCacheDiskCacheEntry** entry,
-      int rv);
-
-  const int64_t resource_id_;
-  base::WeakPtr<AppCacheDiskCache> disk_cache_;
-  AppCacheDiskCacheEntry* entry_ = nullptr;
+  DiskEntryOpener entry_opener_;
 
   // Used to read metadata from disk cache.
   scoped_refptr<BigIOBuffer> metadata_buffer_;
@@ -145,10 +170,6 @@ class ServiceWorkerResourceReaderImpl
   // Helper for ReadData().
   std::unique_ptr<DataReader> data_reader_;
 
-  // Holds the callback of EnsureEntryIsOpen(). Stored as a data member to
-  // handle //net-style maybe-async methods.
-  base::OnceClosure open_entry_callback_;
-
 #if DCHECK_IS_ON()
   enum class State {
     kIdle,
@@ -199,7 +220,7 @@ class ServiceWorkerResourceWriterImpl
                     size_t write_amount,
                     int rv);
 
-  DiskEntryManager entry_manager_;
+  DiskEntryCreator entry_creator_;
 
   // Points the current write position of WriteData().
   size_t write_position_ = 0;
@@ -223,15 +244,12 @@ class ServiceWorkerResourceWriterImpl
 };
 
 // The implementation of storage::mojom::ServiceWorkerResourceMetadataWriter.
-// Currently this class is an adaptor that uses
-// ServiceWorkerResponseMetadataWriter internally.
-// TODO(crbug.com/1055677): Fork the implementation of
-// ServiceWorkerResponseMetadataWriter and stop using it.
 class ServiceWorkerResourceMetadataWriterImpl
     : public storage::mojom::ServiceWorkerResourceMetadataWriter {
  public:
-  explicit ServiceWorkerResourceMetadataWriterImpl(
-      std::unique_ptr<ServiceWorkerResponseMetadataWriter> writer);
+  ServiceWorkerResourceMetadataWriterImpl(
+      int64_t resource_id,
+      base::WeakPtr<AppCacheDiskCache> disk_cache);
 
   ServiceWorkerResourceMetadataWriterImpl(
       const ServiceWorkerResourceMetadataWriterImpl&) = delete;
@@ -240,12 +258,34 @@ class ServiceWorkerResourceMetadataWriterImpl
 
   ~ServiceWorkerResourceMetadataWriterImpl() override;
 
- private:
   // storage::mojom::ServiceWorkerResourceMetadataWriter implementations:
   void WriteMetadata(mojo_base::BigBuffer data,
                      WriteMetadataCallback callback) override;
 
-  const std::unique_ptr<ServiceWorkerResponseMetadataWriter> writer_;
+ private:
+  // Called while executing WriteMetadata().
+  void ContinueWriteMetadata(mojo_base::BigBuffer data,
+                             WriteMetadataCallback callback);
+  void DidWriteMetadata(scoped_refptr<net::IOBuffer> buffer,
+                        size_t write_amount,
+                        int rv);
+
+  DiskEntryOpener entry_opener_;
+
+  // Stored as a data member to handle //net-style maybe-async methods.
+  WriteMetadataCallback write_metadata_callback_;
+
+#if DCHECK_IS_ON()
+  enum class State {
+    kIdle,
+    kWriteMetadataStarted,
+    kWriteMetadataHasEntry,
+  };
+  State state_ = State::kIdle;
+#endif  // DCHECK_IS_ON()
+
+  base::WeakPtrFactory<ServiceWorkerResourceMetadataWriterImpl> weak_factory_{
+      this};
 };
 
 }  // namespace content

content/browser/service_worker/service_worker_storage.cc

@@ -598,10 +598,10 @@ ServiceWorkerStorage::CreateResourceWriter(int64_t resource_id) {
       resource_id, disk_cache()->GetWeakPtr());
 }
 
-std::unique_ptr<ServiceWorkerResponseMetadataWriter>
-ServiceWorkerStorage::CreateResponseMetadataWriter(int64_t resource_id) {
-  return base::WrapUnique(new ServiceWorkerResponseMetadataWriter(
-      resource_id, disk_cache()->GetWeakPtr()));
+std::unique_ptr<ServiceWorkerResourceMetadataWriterImpl>
+ServiceWorkerStorage::CreateResourceMetadataWriter(int64_t resource_id) {
+  return std::make_unique<ServiceWorkerResourceMetadataWriterImpl>(
+      resource_id, disk_cache()->GetWeakPtr());
 }
 
 void ServiceWorkerStorage::StoreUncommittedResourceId(

content/browser/service_worker/service_worker_storage.h

@@ -38,8 +38,6 @@ class QuotaManagerProxy;
 
 namespace content {
 
-class ServiceWorkerDiskCache;
-class ServiceWorkerResponseMetadataWriter;
 class ServiceWorkerStorageControlImplTest;
 
 namespace service_worker_storage_unittest {
@@ -188,8 +186,8 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       int64_t resource_id);
   std::unique_ptr<ServiceWorkerResourceWriterImpl> CreateResourceWriter(
       int64_t resource_id);
-  std::unique_ptr<ServiceWorkerResponseMetadataWriter>
-  CreateResponseMetadataWriter(int64_t resource_id);
+  std::unique_ptr<ServiceWorkerResourceMetadataWriterImpl>
+  CreateResourceMetadataWriter(int64_t resource_id);
 
   // Adds |resource_id| to the set of resources that are in the disk cache
   // but not yet stored with a registration.

content/browser/service_worker/service_worker_storage_control_impl.cc

@@ -272,9 +272,7 @@ void ServiceWorkerStorageControlImpl::CreateResourceMetadataWriter(
         writer) {
   DCHECK_NE(resource_id, blink::mojom::kInvalidServiceWorkerResourceId);
   mojo::MakeSelfOwnedReceiver(
-      std::make_unique<ServiceWorkerResourceMetadataWriterImpl>(
-          storage_->CreateResponseMetadataWriter(resource_id)),
-      std::move(writer));
+      storage_->CreateResourceMetadataWriter(resource_id), std::move(writer));
 }
 
 void ServiceWorkerStorageControlImpl::StoreUncommittedResourceId(