Simplify BlobURLLoader and always send OnStartLoadingResponseBody.

This is a sort of prerequisite for:
https://chromium-review.googlesource.com/c/chromium/src/+/1172290

The main goal is after sending:
  URLLoaderClient::OnReceiveResponse(response_header)
to ALWAYS send:
  URLLoaderClient::OnStartLoadingResponseBody(response_body)

Some URLLoader, when the response's body body is empty, don't even try to send
the response_body data pipe. In the URLLoaderClient, not having to handle the
case with no data pipe would be a nice simplification.

What this CL does:
1) In BlobUrlLoader, create the data pipe at ::Start() time.
2) Pass the producer handle to the MojoBlobCreate at construction time.
3) Pass the consumer handle to OnStartLoadingResponseBody(), immediately
   after OnReceiveResponse().

Some code simplification was possible. There is no more need to
implement PassDataPipe() in BlobURLLoader, ReaderDelegate and
DataPipeReaderDelegate. There is no more need to give them the pipe at
construction time neither.

Bug: 826868, 831155
Change-Id: I827cb39bdd782624ff362874e98be90cab0d47f3
Reviewed-on: https://chromium-review.googlesource.com/c/1329741Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607266}

storage/browser/blob/blob_impl.cc

@@ -21,13 +21,8 @@ namespace {
 
 class ReaderDelegate : public MojoBlobReader::Delegate {
  public:
-  ReaderDelegate(mojo::ScopedDataPipeProducerHandle handle,
-                 blink::mojom::BlobReaderClientPtr client)
-      : handle_(std::move(handle)), client_(std::move(client)) {}
-
-  mojo::ScopedDataPipeProducerHandle PassDataPipe() override {
-    return std::move(handle_);
-  }
+  ReaderDelegate(blink::mojom::BlobReaderClientPtr client)
+      : client_(std::move(client)) {}
 
   MojoBlobReader::Delegate::RequestSideData DidCalculateSize(
       uint64_t total_size,
@@ -43,7 +38,6 @@ class ReaderDelegate : public MojoBlobReader::Delegate {
   }
 
  private:
-  mojo::ScopedDataPipeProducerHandle handle_;
   blink::mojom::BlobReaderClientPtr client_;
 
   DISALLOW_COPY_AND_ASSIGN(ReaderDelegate);
@@ -52,13 +46,8 @@ class ReaderDelegate : public MojoBlobReader::Delegate {
 class DataPipeGetterReaderDelegate : public MojoBlobReader::Delegate {
  public:
   DataPipeGetterReaderDelegate(
-      mojo::ScopedDataPipeProducerHandle handle,
       network::mojom::DataPipeGetter::ReadCallback callback)
-      : handle_(std::move(handle)), callback_(std::move(callback)) {}
-
-  mojo::ScopedDataPipeProducerHandle PassDataPipe() override {
-    return std::move(handle_);
-  }
+      : callback_(std::move(callback)) {}
 
   MojoBlobReader::Delegate::RequestSideData DidCalculateSize(
       uint64_t total_size,
@@ -82,7 +71,6 @@ class DataPipeGetterReaderDelegate : public MojoBlobReader::Delegate {
   }
 
  private:
-  mojo::ScopedDataPipeProducerHandle handle_;
   network::mojom::DataPipeGetter::ReadCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(DataPipeGetterReaderDelegate);
@@ -114,14 +102,14 @@ void BlobImpl::ReadRange(uint64_t offset,
       (length == std::numeric_limits<uint64_t>::max())
           ? net::HttpByteRange::RightUnbounded(offset)
           : net::HttpByteRange::Bounded(offset, offset + length - 1),
-      std::make_unique<ReaderDelegate>(std::move(handle), std::move(client)));
+      std::make_unique<ReaderDelegate>(std::move(client)), std::move(handle));
 }
 
 void BlobImpl::ReadAll(mojo::ScopedDataPipeProducerHandle handle,
                        blink::mojom::BlobReaderClientPtr client) {
-  MojoBlobReader::Create(
-      handle_.get(), net::HttpByteRange(),
-      std::make_unique<ReaderDelegate>(std::move(handle), std::move(client)));
+  MojoBlobReader::Create(handle_.get(), net::HttpByteRange(),
+                         std::make_unique<ReaderDelegate>(std::move(client)),
+                         std::move(handle));
 }
 
 void BlobImpl::ReadSideData(ReadSideDataCallback callback) {
@@ -187,11 +175,12 @@ void BlobImpl::Clone(network::mojom::DataPipeGetterRequest request) {
   data_pipe_getter_bindings_.AddBinding(this, std::move(request));
 }
 
-void BlobImpl::Read(mojo::ScopedDataPipeProducerHandle pipe,
+void BlobImpl::Read(mojo::ScopedDataPipeProducerHandle handle,
                     ReadCallback callback) {
-  MojoBlobReader::Create(handle_.get(), net::HttpByteRange(),
-                         std::make_unique<DataPipeGetterReaderDelegate>(
-                             std::move(pipe), std::move(callback)));
+  MojoBlobReader::Create(
+      handle_.get(), net::HttpByteRange(),
+      std::make_unique<DataPipeGetterReaderDelegate>(std::move(callback)),
+      std::move(handle));
 }
 
 void BlobImpl::FlushForTesting() {

storage/browser/blob/blob_url_loader.cc

@@ -88,8 +88,12 @@ void BlobURLLoader::Start(const network::ResourceRequest& request) {
     }
   }
 
+  mojo::DataPipe data_pipe(kDefaultAllocationSize);
+  response_body_consumer_handle_ = std::move(data_pipe.consumer_handle);
+
   MojoBlobReader::Create(blob_handle_.get(), byte_range_,
-                         base::WrapUnique(this));
+                         base::WrapUnique(this),
+                         std::move(data_pipe.producer_handle));
 }
 
 void BlobURLLoader::FollowRedirect(
@@ -99,12 +103,6 @@ void BlobURLLoader::FollowRedirect(
   NOTREACHED();
 }
 
-mojo::ScopedDataPipeProducerHandle BlobURLLoader::PassDataPipe() {
-  mojo::DataPipe data_pipe(kDefaultAllocationSize);
-  response_body_consumer_handle_ = std::move(data_pipe.consumer_handle);
-  return std::move(data_pipe.producer_handle);
-}
-
 MojoBlobReader::Delegate::RequestSideData BlobURLLoader::DidCalculateSize(
     uint64_t total_size,
     uint64_t content_size) {
@@ -130,14 +128,6 @@ void BlobURLLoader::DidReadSideData(net::IOBufferWithSize* data) {
   HeadersCompleted(net::HTTP_OK, total_size_, data);
 }
 
-void BlobURLLoader::DidRead(int num_bytes) {
-  if (response_body_consumer_handle_.is_valid()) {
-    // Send the data pipe on the first OnReadCompleted call.
-    client_->OnStartLoadingResponseBody(
-        std::move(response_body_consumer_handle_));
-  }
-}
-
 void BlobURLLoader::OnComplete(net::Error error_code,
                                uint64_t total_written_bytes) {
   network::URLLoaderCompletionStatus status(error_code);
@@ -172,6 +162,9 @@ void BlobURLLoader::HeadersCompleted(net::HttpStatusCode status_code,
     client_->OnReceiveCachedMetadata(
         std::vector<uint8_t>(data, data + metadata->size()));
   }
+
+  client_->OnStartLoadingResponseBody(
+      std::move(response_body_consumer_handle_));
 }
 
 }  // namespace storage

storage/browser/blob/blob_url_loader.h

@@ -50,11 +50,9 @@ class STORAGE_EXPORT BlobURLLoader : public storage::MojoBlobReader::Delegate,
   void ResumeReadingBodyFromNet() override {}
 
   // storage::MojoBlobReader::Delegate implementation:
-  mojo::ScopedDataPipeProducerHandle PassDataPipe() override;
   RequestSideData DidCalculateSize(uint64_t total_size,
                                    uint64_t content_size) override;
   void DidReadSideData(net::IOBufferWithSize* data) override;
-  void DidRead(int num_bytes) override;
   void OnComplete(net::Error error_code, uint64_t total_written_bytes) override;
 
   void HeadersCompleted(net::HttpStatusCode status_code,

storage/browser/blob/mojo_blob_reader.cc

@@ -13,18 +13,24 @@
 namespace storage {
 
 // static
-void MojoBlobReader::Create(const BlobDataHandle* handle,
-                            const net::HttpByteRange& range,
-                            std::unique_ptr<Delegate> delegate) {
-  new MojoBlobReader(handle, range, std::move(delegate));
+void MojoBlobReader::Create(
+    const BlobDataHandle* handle,
+    const net::HttpByteRange& range,
+    std::unique_ptr<Delegate> delegate,
+    mojo::ScopedDataPipeProducerHandle response_body_stream) {
+  new MojoBlobReader(handle, range, std::move(delegate),
+                     std::move(response_body_stream));
 }
 
-MojoBlobReader::MojoBlobReader(const BlobDataHandle* handle,
-                               const net::HttpByteRange& range,
-                               std::unique_ptr<Delegate> delegate)
+MojoBlobReader::MojoBlobReader(
+    const BlobDataHandle* handle,
+    const net::HttpByteRange& range,
+    std::unique_ptr<Delegate> delegate,
+    mojo::ScopedDataPipeProducerHandle response_body_stream)
     : delegate_(std::move(delegate)),
       byte_range_(range),
       blob_reader_(handle->CreateReader()),
+      response_body_stream_(std::move(response_body_stream)),
       writable_handle_watcher_(FROM_HERE,
                                mojo::SimpleWatcher::ArmingPolicy::MANUAL,
                                base::SequencedTaskRunnerHandle::Get()),
@@ -82,9 +88,12 @@ void MojoBlobReader::NotifyCompletedAndDeleteIfNeeded(int result) {
     notified_completed_ = true;
   }
 
-  bool has_data_pipe = pending_write_ || response_body_stream_.is_valid();
-  if (!has_data_pipe)
-    delete this;
+  // If data are being written, wait for it to complete.
+  if (writable_handle_watcher_.IsWatching() &&
+      (pending_write_ || response_body_stream_.is_valid()))
+    return;
+
+  delete this;
 }
 
 void MojoBlobReader::DidCalculateSize(int result) {
@@ -147,9 +156,7 @@ void MojoBlobReader::DidReadSideData(BlobReader::Status status) {
 
 void MojoBlobReader::StartReading() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!response_body_stream_);
 
-  response_body_stream_ = delegate_->PassDataPipe();
   peer_closed_handle_watcher_.Watch(
       response_body_stream_.get(), MOJO_HANDLE_SIGNAL_PEER_CLOSED,
       MOJO_WATCH_CONDITION_SATISFIED,

storage/browser/blob/mojo_blob_reader.h

@@ -58,11 +58,6 @@ class STORAGE_EXPORT MojoBlobReader {
     // data this method is called with null.
     virtual void DidReadSideData(net::IOBufferWithSize* data) {}
 
-    // Called when the MojoBlobReader actually starts reading data from the
-    // blob. Should return a data pipe to which all the data read from the blob
-    // should be written.
-    virtual mojo::ScopedDataPipeProducerHandle PassDataPipe() = 0;
-
     // Called whenever some amount of data is read from the blob and about to be
     // written to the data pipe.
     virtual void DidRead(int num_bytes) {}
@@ -80,12 +75,14 @@ class STORAGE_EXPORT MojoBlobReader {
 
   static void Create(const BlobDataHandle* handle,
                      const net::HttpByteRange& range,
-                     std::unique_ptr<Delegate> delegate);
+                     std::unique_ptr<Delegate> delegate,
+                     mojo::ScopedDataPipeProducerHandle response_body_stream);
 
  private:
   MojoBlobReader(const BlobDataHandle* handle,
                  const net::HttpByteRange& range,
-                 std::unique_ptr<Delegate> delegate);
+                 std::unique_ptr<Delegate> delegate,
+                 mojo::ScopedDataPipeProducerHandle response_body_stream);
   ~MojoBlobReader();
 
   void Start();