Revert "Reland: URLRequestJob: change ReadRawData contract"

This reverts commit d77911ac82186d65a8f11555a7a6b1678c769ba2.

The previous CL caused a top crasher on Canary.
Reverting this CL now since fixing the crash isn't
straightforward.

TBR=michaeln@chromium.org,mnaganov@chromium.org,skyostil@chromium.org,eugenebut@chromium.org,davidben@chromium.org,falken@chromium.org,mtomasz@chromium.org, sky@chromium.org,jianli@chromium.org,zork@chromium.org,mmenke@chromium.org,rdsmith@chromium.org

BUG=558224
BUG=553300
BUG=474859
BUG=329902

Review URL: https://codereview.chromium.org/1459333002

Cr-Commit-Position: refs/heads/master@{#360809}

6 files changed, 84 insertions, 58 deletions

diff --git a/storage/browser/blob/blob_url_request_job.cc b/storage/browser/blob/blob_url_request_job.cc
index deea300..6ec2fe5 100644
--- a/storage/browser/blob/blob_url_request_job.cc
+++ b/storage/browser/blob/blob_url_request_job.cc
@@ -75,37 +75,41 @@ void BlobURLRequestJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-int BlobURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) {
+bool BlobURLRequestJob::ReadRawData(net::IOBuffer* dest,
+                                    int dest_size,
+                                    int* bytes_read) {
   TRACE_EVENT_ASYNC_BEGIN1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                            blob_handle_ ? blob_handle_->uuid() : "NotFound");
   DCHECK_NE(dest_size, 0);
+  DCHECK(bytes_read);
 
-  // Bail out immediately if we encounter an error. This happens if a previous
-  // ReadRawData signalled an error to its caller but the caller called
-  // ReadRawData again anyway.
-  if (error_)
-    return 0;
+  // Bail out immediately if we encounter an error.
+  if (error_) {
+    *bytes_read = 0;
+    return true;
+  }
 
-  int bytes_read = 0;
   BlobReader::Status read_status =
-      blob_reader_->Read(dest, dest_size, &bytes_read,
+      blob_reader_->Read(dest, dest_size, bytes_read,
                          base::Bind(&BlobURLRequestJob::DidReadRawData,
                                     weak_factory_.GetWeakPtr()));
 
   switch (read_status) {
     case BlobReader::Status::NET_ERROR:
+      NotifyFailure(blob_reader_->net_error());
       TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                              blob_handle_ ? blob_handle_->uuid() : "NotFound");
-      return blob_reader_->net_error();
+      return false;
     case BlobReader::Status::IO_PENDING:
-      return net::ERR_IO_PENDING;
+      SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+      return false;
     case BlobReader::Status::DONE:
       TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                              blob_handle_ ? blob_handle_->uuid() : "NotFound");
-      return bytes_read;
+      return true;
   }
   NOTREACHED();
-  return 0;
+  return true;
 }
 
 bool BlobURLRequestJob::GetMimeType(std::string* mime_type) const {
@@ -218,7 +222,13 @@ void BlobURLRequestJob::DidCalculateSize(int result) {
 void BlobURLRequestJob::DidReadRawData(int result) {
   TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                          blob_handle_ ? blob_handle_->uuid() : "NotFound");
-  ReadRawDataComplete(result);
+  if (result < 0) {
+    NotifyFailure(result);
+    return;
+  }
+  // Clear the IO_PENDING status
+  SetStatus(net::URLRequestStatus());
+  NotifyReadComplete(result);
 }
 
 void BlobURLRequestJob::NotifyFailure(int error_code) {
@@ -226,7 +236,11 @@ void BlobURLRequestJob::NotifyFailure(int error_code) {
 
   // If we already return the headers on success, we can't change the headers
   // now. Instead, we just error out.
-  DCHECK(!response_info_) << "Cannot NotifyFailure after headers.";
+  if (response_info_) {
+    NotifyDone(
+        net::URLRequestStatus(net::URLRequestStatus::FAILED, error_code));
+    return;
+  }
 
   net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR;
   switch (error_code) {
diff --git a/storage/browser/blob/blob_url_request_job.h b/storage/browser/blob/blob_url_request_job.h
index 1f0b9fb..21baa2c 100644
--- a/storage/browser/blob/blob_url_request_job.h
+++ b/storage/browser/blob/blob_url_request_job.h
@@ -44,7 +44,7 @@ class STORAGE_EXPORT BlobURLRequestJob
   // net::URLRequestJob methods.
   void Start() override;
   void Kill() override;
-  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
+  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
   bool GetMimeType(std::string* mime_type) const override;
   void GetResponseInfo(net::HttpResponseInfo* info) override;
   int GetResponseCode() const override;
diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.cc b/storage/browser/fileapi/file_system_dir_url_request_job.cc
index c053773..fc91f3b 100644
--- a/storage/browser/fileapi/file_system_dir_url_request_job.cc
+++ b/storage/browser/fileapi/file_system_dir_url_request_job.cc
@@ -14,6 +14,7 @@
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/url_request/url_request.h"
 #include "storage/browser/fileapi/file_system_context.h"
@@ -43,14 +44,16 @@ FileSystemDirURLRequestJob::FileSystemDirURLRequestJob(
 FileSystemDirURLRequestJob::~FileSystemDirURLRequestJob() {
 }
 
-int FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest,
-                                            int dest_size) {
-  int count = std::min(dest_size, base::checked_cast<int>(data_.size()));
+bool FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest,
+                                             int dest_size,
+                                             int* bytes_read) {
+  int count = std::min(dest_size, static_cast<int>(data_.size()));
   if (count > 0) {
     memcpy(dest->data(), data_.data(), count);
     data_.erase(0, count);
   }
-  return count;
+  *bytes_read = count;
+  return true;
 }
 
 void FileSystemDirURLRequestJob::Start() {
@@ -96,7 +99,8 @@ void FileSystemDirURLRequestJob::StartAsync() {
                        false);
       return;
     }
-    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
+    NotifyDone(
+        URLRequestStatus(URLRequestStatus::FAILED, net::ERR_FILE_NOT_FOUND));
     return;
   }
   file_system_context_->operation_runner()->ReadDirectory(
@@ -109,7 +113,8 @@ void FileSystemDirURLRequestJob::DidAttemptAutoMount(base::File::Error result) {
       file_system_context_->CrackURL(request_->url()).is_valid()) {
     StartAsync();
   } else {
-    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
+    NotifyDone(
+        URLRequestStatus(URLRequestStatus::FAILED, net::ERR_FILE_NOT_FOUND));
   }
 }
 
@@ -121,7 +126,7 @@ void FileSystemDirURLRequestJob::DidReadDirectory(
     int rv = net::ERR_FILE_NOT_FOUND;
     if (result == base::File::FILE_ERROR_INVALID_URL)
       rv = net::ERR_INVALID_URL;
-    NotifyStartError(URLRequestStatus::FromError(rv));
+    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
     return;
   }
 
@@ -170,7 +175,7 @@ void FileSystemDirURLRequestJob::DidGetMetadata(
     int rv = net::ERR_FILE_NOT_FOUND;
     if (result == base::File::FILE_ERROR_INVALID_URL)
       rv = net::ERR_INVALID_URL;
-    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, rv));
+    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
   }
 
   if (!request_)
diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.h b/storage/browser/fileapi/file_system_dir_url_request_job.h
index 93fdd1f..01bd372 100644
--- a/storage/browser/fileapi/file_system_dir_url_request_job.h
+++ b/storage/browser/fileapi/file_system_dir_url_request_job.h
@@ -32,7 +32,7 @@ class STORAGE_EXPORT FileSystemDirURLRequestJob : public net::URLRequestJob {
   // URLRequestJob methods:
   void Start() override;
   void Kill() override;
-  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
+  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
   bool GetCharset(std::string* charset) override;
 
   // FilterContext methods (via URLRequestJob):
diff --git a/storage/browser/fileapi/file_system_url_request_job.cc b/storage/browser/fileapi/file_system_url_request_job.cc
index 8edcb3d..8d58f64 100644
--- a/storage/browser/fileapi/file_system_url_request_job.cc
+++ b/storage/browser/fileapi/file_system_url_request_job.cc
@@ -62,7 +62,6 @@ FileSystemURLRequestJob::FileSystemURLRequestJob(
       file_system_context_(file_system_context),
       is_directory_(false),
       remaining_bytes_(0),
-      range_parse_result_(net::OK),
       weak_factory_(this) {}
 
 FileSystemURLRequestJob::~FileSystemURLRequestJob() {}
@@ -80,28 +79,39 @@ void FileSystemURLRequestJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-int FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) {
+bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest,
+                                          int dest_size,
+                                          int* bytes_read) {
   DCHECK_NE(dest_size, 0);
+  DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
   if (reader_.get() == NULL)
-    return net::ERR_FAILED;
+    return false;
 
   if (remaining_bytes_ < dest_size)
-    dest_size = remaining_bytes_;
+    dest_size = static_cast<int>(remaining_bytes_);
 
-  if (!dest_size)
-    return 0;
+  if (!dest_size) {
+    *bytes_read = 0;
+    return true;
+  }
 
   const int rv = reader_->Read(dest, dest_size,
                                base::Bind(&FileSystemURLRequestJob::DidRead,
                                           weak_factory_.GetWeakPtr()));
   if (rv >= 0) {
+    // Data is immediately available.
+    *bytes_read = rv;
     remaining_bytes_ -= rv;
     DCHECK_GE(remaining_bytes_, 0);
+    return true;
   }
-
-  return rv;
+  if (rv == net::ERR_IO_PENDING)
+    SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
+  else
+    NotifyFailed(rv);
+  return false;
 }
 
 bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const {
@@ -116,12 +126,8 @@ bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const {
 void FileSystemURLRequestJob::SetExtraRequestHeaders(
     const net::HttpRequestHeaders& headers) {
   std::string range_header;
-  // Currently this job only cares about the Range header. Note that validation
-  // is deferred to DidGetMetaData(), because NotifyStartError is not legal to
-  // call since the job has not started.
   if (headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header)) {
     std::vector<net::HttpByteRange> ranges;
-
     if (net::HttpUtil::ParseRangeHeader(range_header, &ranges)) {
       if (ranges.size() == 1) {
         byte_range_ = ranges[0];
@@ -129,7 +135,7 @@ void FileSystemURLRequestJob::SetExtraRequestHeaders(
         // We don't support multiple range requests in one single URL request.
         // TODO(adamk): decide whether we want to support multiple range
         // requests.
-        range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
+        NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
       }
     }
   }
@@ -161,7 +167,7 @@ void FileSystemURLRequestJob::StartAsync() {
   }
   if (!file_system_context_->CanServeURLRequest(url_)) {
     // In incognito mode the API is not usable and there should be no data.
-    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
+    NotifyFailed(net::ERR_FILE_NOT_FOUND);
     return;
   }
   file_system_context_->operation_runner()->GetMetadata(
@@ -176,7 +182,7 @@ void FileSystemURLRequestJob::DidAttemptAutoMount(base::File::Error result) {
       file_system_context_->CrackURL(request_->url()).is_valid()) {
     StartAsync();
   } else {
-    NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND));
+    NotifyFailed(net::ERR_FILE_NOT_FOUND);
   }
 }
 
@@ -184,10 +190,9 @@ void FileSystemURLRequestJob::DidGetMetadata(
     base::File::Error error_code,
     const base::File::Info& file_info) {
   if (error_code != base::File::FILE_OK) {
-    NotifyStartError(URLRequestStatus::FromError(
-        error_code == base::File::FILE_ERROR_INVALID_URL
-            ? net::ERR_INVALID_URL
-            : net::ERR_FILE_NOT_FOUND));
+    NotifyFailed(error_code == base::File::FILE_ERROR_INVALID_URL
+                     ? net::ERR_INVALID_URL
+                     : net::ERR_FILE_NOT_FOUND);
     return;
   }
 
@@ -197,14 +202,8 @@ void FileSystemURLRequestJob::DidGetMetadata(
 
   is_directory_ = file_info.is_directory;
 
-  if (range_parse_result_ != net::OK) {
-    NotifyStartError(URLRequestStatus::FromError(range_parse_result_));
-    return;
-  }
-
   if (!byte_range_.ComputeBounds(file_info.size)) {
-    NotifyStartError(
-        URLRequestStatus::FromError(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+    NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
     return;
   }
 
@@ -228,12 +227,17 @@ void FileSystemURLRequestJob::DidGetMetadata(
 }
 
 void FileSystemURLRequestJob::DidRead(int result) {
-  if (result >= 0) {
-    remaining_bytes_ -= result;
-    DCHECK_GE(remaining_bytes_, 0);
-  }
+  if (result > 0)
+    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
+  else if (result == 0)
+    NotifyDone(URLRequestStatus());
+  else
+    NotifyFailed(result);
+
+  remaining_bytes_ -= result;
+  DCHECK_GE(remaining_bytes_, 0);
 
-  ReadRawDataComplete(result);
+  NotifyReadComplete(result);
 }
 
 bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location,
@@ -253,4 +257,8 @@ bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location,
   return false;
 }
 
+void FileSystemURLRequestJob::NotifyFailed(int rv) {
+  NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
+}
+
 }  // namespace storage
diff --git a/storage/browser/fileapi/file_system_url_request_job.h b/storage/browser/fileapi/file_system_url_request_job.h
index 308b66c..e442cae 100644
--- a/storage/browser/fileapi/file_system_url_request_job.h
+++ b/storage/browser/fileapi/file_system_url_request_job.h
@@ -11,7 +11,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "net/base/net_errors.h"
 #include "net/http/http_byte_range.h"
 #include "net/url_request/url_request_job.h"
 #include "storage/browser/fileapi/file_system_url.h"
@@ -42,7 +41,7 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob {
   // URLRequestJob methods:
   void Start() override;
   void Kill() override;
-  int ReadRawData(net::IOBuffer* buf, int buf_size) override;
+  bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override;
   bool IsRedirectResponse(GURL* location, int* http_status_code) override;
   void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers) override;
   void GetResponseInfo(net::HttpResponseInfo* info) override;
@@ -61,6 +60,7 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob {
   void DidGetMetadata(base::File::Error error_code,
                       const base::File::Info& file_info);
   void DidRead(int result);
+  void NotifyFailed(int rv);
 
   const std::string storage_domain_;
   FileSystemContext* file_system_context_;
@@ -69,7 +69,6 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob {
   bool is_directory_;
   scoped_ptr<net::HttpResponseInfo> response_info_;
   int64 remaining_bytes_;
-  net::Error range_parse_result_;
   net::HttpByteRange byte_range_;
   base::WeakPtrFactory<FileSystemURLRequestJob> weak_factory_;