Refactor ResourceHandler API.

content/browser/loader/redirect_to_file_resource_handler.cc

Index: content/browser/loader/redirect_to_file_resource_handler.cc
diff --git a/content/browser/loader/redirect_to_file_resource_handler.cc b/content/browser/loader/redirect_to_file_resource_handler.cc
index 2d55420e95a9f62ef659daf9e3bb6f8f56a63da6..beda724f08b2ec77b53e38b6cb19b4f0a314f32a 100644
--- a/content/browser/loader/redirect_to_file_resource_handler.cc
+++ b/content/browser/loader/redirect_to_file_resource_handler.cc
@@ -138,7 +138,6 @@ RedirectToFileResourceHandler::RedirectToFileResourceHandler(
       write_cursor_(0),
       writer_(NULL),
       next_buffer_size_(kInitialReadBufSize),
-      did_defer_(false),
       completed_during_write_(false),
       weak_factory_(this) {}
 
@@ -156,22 +155,24 @@ void RedirectToFileResourceHandler::
   create_temporary_file_stream_ = create_temporary_file_stream;
 }
 
-bool RedirectToFileResourceHandler::OnResponseStarted(
+void RedirectToFileResourceHandler::OnResponseStarted(
     ResourceResponse* response,
-    bool* defer) {
+    std::unique_ptr<ResourceController> controller) {
   DCHECK(writer_);
   response->head.download_file_path = writer_->path();
-  return next_handler_->OnResponseStarted(response, defer);
+  next_handler_->OnResponseStarted(response, std::move(controller));
 }
 
-bool RedirectToFileResourceHandler::OnWillStart(const GURL& url, bool* defer) {
+void RedirectToFileResourceHandler::OnWillStart(
+    const GURL& url,
+    std::unique_ptr<ResourceController> controller) {
   DCHECK(!writer_);
 
   // Defer starting the request until we have created the temporary file.
   // TODO(darin): This is sub-optimal.  We should not delay starting the
   // network request like this.
   will_start_url_ = url;
-  did_defer_ = *defer = true;
+  set_controller(std::move(controller));
   if (create_temporary_file_stream_.is_null()) {
     CreateTemporaryFileStream(
         base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
@@ -181,7 +182,6 @@ bool RedirectToFileResourceHandler::OnWillStart(const GURL& url, bool* defer) {
         base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
                    weak_factory_.GetWeakPtr()));
   }
-  return true;
 }
 
 bool RedirectToFileResourceHandler::OnWillRead(
@@ -203,8 +203,9 @@ bool RedirectToFileResourceHandler::OnWillRead(
   return true;
 }
 
-bool RedirectToFileResourceHandler::OnReadCompleted(int bytes_read,
-                                                    bool* defer) {
+void RedirectToFileResourceHandler::OnReadCompleted(
+    int bytes_read,
+    std::unique_ptr<ResourceController> controller) {
   DCHECK(buf_write_pending_);
   buf_write_pending_ = false;
 
@@ -213,30 +214,35 @@ bool RedirectToFileResourceHandler::OnReadCompleted(int bytes_read,
   DCHECK(new_offset <= buf_->capacity());
   buf_->set_offset(new_offset);
 
+  if (!WriteMore()) {

[Randy Smith (Not in Mondays), 2016/12/31 22:00:25]

Why the move before BufIsFull()?  It looks like a functional change (albeit a
good one, as it ups the chances of the buffer not being full).

[mmenke, 2017/01/06 18:46:58]

Because it makes control flow simpler.  I really don't like:

if (BufIsFull()) {
  set_controller(std::move(controller));
  ...
}

if (!WriteMore()) {
  if (has_controller()) {
    controller_->Cancel();
  } else {
    controller->Cancel();
  }
}

if (!has_controller())
  controller->Resume();

(Note that because std::move is odd, have to check has_controller() instead of
controller, though even if that wasn't the case, this would still be really
ugly)

+    controller->Cancel();
+    return;
+  }
+
   if (BufIsFull()) {
-    did_defer_ = *defer = true;
+    set_controller(std::move(controller));
 
     if (buf_->capacity() == bytes_read) {
       // The network layer has saturated our buffer in one read. Next time, we
       // should give it a bigger buffer for it to fill.
       next_buffer_size_ = std::min(next_buffer_size_ * 2, kMaxReadBufSize);
     }
+    return;
   }
 
-  return WriteMore();
+  controller->Resume();
 }
 
 void RedirectToFileResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
-    bool* defer) {
+    std::unique_ptr<ResourceController> controller) {
   if (writer_ && writer_->is_writing()) {
     completed_during_write_ = true;
     completed_status_ = status;
-    did_defer_ = true;
-    *defer = true;
+    set_controller(std::move(controller));
     return;
   }
-  next_handler_->OnResponseCompleted(status, defer);
+  next_handler_->OnResponseCompleted(status, std::move(controller));
 }
 
 void RedirectToFileResourceHandler::DidCreateTemporaryFile(
@@ -244,24 +250,16 @@ void RedirectToFileResourceHandler::DidCreateTemporaryFile(
     std::unique_ptr<net::FileStream> file_stream,
     ShareableFileReference* deletable_file) {
   DCHECK(!writer_);
+  DCHECK(has_controller());
   if (error_code != base::File::FILE_OK) {
-    controller()->CancelWithError(net::FileErrorToNetError(error_code));
+    CancelWithError(net::FileErrorToNetError(error_code));
     return;
   }
 
   writer_ = new Writer(this, std::move(file_stream), deletable_file);
 
   // Resume the request.
-  DCHECK(did_defer_);
-  bool defer = false;
-  if (!next_handler_->OnWillStart(will_start_url_, &defer)) {
-    controller()->Cancel();
-  } else if (!defer) {
-    ResumeIfDeferred();
-  } else {
-    did_defer_ = false;
-  }
-  will_start_url_ = GURL();
+  next_handler_->OnWillStart(std::move(will_start_url_), TakeController());
 }
 
 void RedirectToFileResourceHandler::DidWriteToFile(int result) {
@@ -284,21 +282,27 @@ void RedirectToFileResourceHandler::DidWriteToFile(int result) {
       completed_status_ = net::URLRequestStatus(net::URLRequestStatus::CANCELED,
                                                 net::ERR_FAILED);
     }
-    if (!completed_during_write_)
-      controller()->CancelWithError(net::ERR_FAILED);
+    if (!completed_during_write_) {
+      // TODO(mmenke):  Should out of band cancellation be able to set an error
+      // code? This case used to return ERR_FAILED. All other out of band
+      // cancels are aborts.
+      if (has_controller()) {
+        // If the write buffer is full, |this| has deferred the request, and
+        // can do an in-band cancel.
+        Cancel();
+      } else {
+        OutOfBandCancel(net::ERR_FAILED, true);
+      }
+      return;
+    }
   }
 
   if (completed_during_write_ && !writer_->is_writing()) {
     // Resume shutdown now that all data has been written to disk. Note that
     // this should run even in the |failed| case above, otherwise a failed write
     // leaves the handler stuck.
-    bool defer = false;
-    next_handler_->OnResponseCompleted(completed_status_, &defer);
-    if (!defer) {
-      ResumeIfDeferred();
-    } else {
-      did_defer_ = false;
-    }
+    DCHECK(has_controller());
+    next_handler_->OnResponseCompleted(completed_status_, TakeController());
   }
 }
 
@@ -357,10 +361,8 @@ bool RedirectToFileResourceHandler::BufIsFull() const {
 }
 
 void RedirectToFileResourceHandler::ResumeIfDeferred() {
-  if (did_defer_) {
-    did_defer_ = false;
-    controller()->Resume();
-  }
+  if (has_controller())
+    Resume();
 }
 
 }  // namespace content