URLRequestJob: change ReadRawData contract

storage/browser/blob/blob_url_request_job.cc

Index: storage/browser/blob/blob_url_request_job.cc
diff --git a/storage/browser/blob/blob_url_request_job.cc b/storage/browser/blob/blob_url_request_job.cc
index 6ec2fe57ae7ca706b53171c9d8e1f8a24f1d3d3f..deea30027a321abe8f06f4cf5038431685d2a1ea 100644
--- a/storage/browser/blob/blob_url_request_job.cc
+++ b/storage/browser/blob/blob_url_request_job.cc
@@ -75,41 +75,37 @@ void BlobURLRequestJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
-bool BlobURLRequestJob::ReadRawData(net::IOBuffer* dest,
-                                    int dest_size,
-                                    int* bytes_read) {
+int BlobURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) {
   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.
-  if (error_) {
-    *bytes_read = 0;
-    return true;
-  }
+  // 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;
 
+  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,

[jianli, 2015/11/05 23:55:23]

Why not passing ReadRawDataComplete directly such that
BlobURLRequestJob::DidReadRawData can be removed?

[michaeln, 2015/11/06 00:10:51]

how would you end the async trace w/o the intermediary?

[mmenke, 2015/11/06 00:52:34]

You can use weak ptrs with parent class methods, without problems, I believe.

[michaeln, 2015/11/06 01:22:39]

Sure, but the parent class method won't do this...
TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid"..

[mmenke, 2015/11/06 01:27:47]

Sorry...Missed the "trace."  Saw "end the async event", and thought you were
talking about cancellation.

[xunjieli, 2015/11/06 14:30:03]

Acknowledged. Looks like we still would like to keep the intermediary in order
to properly end the traces. So no action is needed on my end.

                                     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 false;
+      return blob_reader_->net_error();
     case BlobReader::Status::IO_PENDING:
-      SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
-      return false;
+      return net::ERR_IO_PENDING;
     case BlobReader::Status::DONE:
       TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid",
                              blob_handle_ ? blob_handle_->uuid() : "NotFound");
-      return true;
+      return bytes_read;
   }
   NOTREACHED();
-  return true;
+  return 0;
 }
 
 bool BlobURLRequestJob::GetMimeType(std::string* mime_type) const {
@@ -222,13 +218,7 @@ 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");
-  if (result < 0) {
-    NotifyFailure(result);
-    return;
-  }
-  // Clear the IO_PENDING status
-  SetStatus(net::URLRequestStatus());
-  NotifyReadComplete(result);
+  ReadRawDataComplete(result);
 }
 
 void BlobURLRequestJob::NotifyFailure(int error_code) {
@@ -236,11 +226,7 @@ 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.
-  if (response_info_) {
-    NotifyDone(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, error_code));
-    return;
-  }
+  DCHECK(!response_info_) << "Cannot NotifyFailure after headers.";
 
   net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR;
   switch (error_code) {