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 7ddcdded6b374369bf5c64b16c58e5ba4c2ca12b..ffc93752fe0b4bf1f446a41d5cafa92fc57905ef 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;
 
@@ -214,7 +215,7 @@ bool RedirectToFileResourceHandler::OnReadCompleted(int bytes_read,
   buf_->set_offset(new_offset);
 
   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
@@ -223,20 +224,26 @@ bool RedirectToFileResourceHandler::OnReadCompleted(int bytes_read,
     }
   }
 
-  return WriteMore();
+  if (!WriteMore()) {
+    if (controller) {
+      controller->Cancel();
+    } else {
+      // Note that |controller_| can't be nullptr here.

[Randy Smith (Not in Mondays), 2016/12/16 21:37:26]

I don't follow this comment; it looks like controller_ *is* nullptr here.

[mmenke, 2016/12/22 16:29:35]

|controller| vs |controller_|.  If a read completes, one or the other is not
nullptr.  (i.e., if this is the BufIsFull() path, we've set controller_ but not
destroyed it/used it in WriteMore()).

+      Cancel();
+    }
+  }
 }
 
 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 +251,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 +283,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();
+      }
+      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 +362,8 @@ bool RedirectToFileResourceHandler::BufIsFull() const {
 }
 
 void RedirectToFileResourceHandler::ResumeIfDeferred() {
-  if (did_defer_) {
-    did_defer_ = false;
-    controller()->Resume();
-  }
+  if (has_controller())
+    Resume();
 }
 
 }  // namespace content