Reland: URLRequestJob: change ReadRawData contract

content/browser/streams/stream_url_request_job.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/streams/stream_url_request_job.h"
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 22 matching lines @@
       weak_factory_(this) {
   DCHECK(stream_.get());
   stream_->SetReadObserver(this);
 }
 
 StreamURLRequestJob::~StreamURLRequestJob() {
   ClearStream();
 }
 
 void StreamURLRequestJob::OnDataAvailable(Stream* stream) {
-  // Clear the IO_PENDING status.
-  SetStatus(net::URLRequestStatus());
   // Do nothing if pending_buffer_ is empty, i.e. there's no ReadRawData()
   // operation waiting for IO completion.
   if (!pending_buffer_.get())
     return;
 
   // pending_buffer_ is set to the IOBuffer instance provided to ReadRawData()
   // by URLRequestJob.
 
-  int bytes_read;
+  int result = 0;
   switch (stream_->ReadRawData(pending_buffer_.get(), pending_buffer_size_,
-                               &bytes_read)) {
+                               &result)) {
     case Stream::STREAM_HAS_DATA:
-      DCHECK_GT(bytes_read, 0);
+      DCHECK_GT(result, 0);
       break;
     case Stream::STREAM_COMPLETE:
-      // Ensure this. Calling NotifyReadComplete call with 0 signals
-      // completion.
-      bytes_read = 0;
+      // Ensure ReadRawData gives net::OK.
+      DCHECK_EQ(net::OK, result);
       break;
     case Stream::STREAM_EMPTY:
       NOTREACHED();
       break;
     case Stream::STREAM_ABORTED:
       // Handle this as connection reset.
-      NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                       net::ERR_CONNECTION_RESET));
+      result = net::ERR_CONNECTION_RESET;
       break;
   }
 
   // Clear the buffers before notifying the read is complete, so that it is
   // safe for the observer to read.
   pending_buffer_ = NULL;
   pending_buffer_size_ = 0;
 
-  total_bytes_read_ += bytes_read;
-  NotifyReadComplete(bytes_read);
+  if (result > 0)
+    total_bytes_read_ += result;
+  ReadRawDataComplete(result);
 }
 
 // net::URLRequestJob methods.
 void StreamURLRequestJob::Start() {
   // Continue asynchronously.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&StreamURLRequestJob::DidStart, weak_factory_.GetWeakPtr()));
 }
 
 void StreamURLRequestJob::Kill() {
   net::URLRequestJob::Kill();
   weak_factory_.InvalidateWeakPtrs();
   ClearStream();
 }
 
-bool StreamURLRequestJob::ReadRawData(net::IOBuffer* buf,
-                                      int buf_size,
-                                      int* bytes_read) {
+int StreamURLRequestJob::ReadRawData(net::IOBuffer* buf, int buf_size) {
+  // TODO(ellyjones): This is not right. The old code returned true here, but
+  // ReadRawData's old contract was to return true only for synchronous
+  // successes, which had the effect of treating all errors as synchronous EOFs.
+  // See https://crbug.com/508957
   if (request_failed_)
-    return true;
+    return 0;
 
   DCHECK(buf);
-  DCHECK(bytes_read);
   int to_read = buf_size;
   if (max_range_ && to_read) {
     if (to_read + total_bytes_read_ > max_range_)
       to_read = max_range_ - total_bytes_read_;
 
-    if (to_read <= 0) {
-      *bytes_read = 0;
-      return true;
-    }
+    if (to_read == 0)
+      return 0;
   }
 
-  switch (stream_->ReadRawData(buf, to_read, bytes_read)) {
+  int bytes_read = 0;
+  switch (stream_->ReadRawData(buf, to_read, &bytes_read)) {
     case Stream::STREAM_HAS_DATA:
     case Stream::STREAM_COMPLETE:
-      total_bytes_read_ += *bytes_read;
-      return true;
+      total_bytes_read_ += bytes_read;
+      return bytes_read;
     case Stream::STREAM_EMPTY:
       pending_buffer_ = buf;
       pending_buffer_size_ = to_read;
-      SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
-      return false;
+      return net::ERR_IO_PENDING;
     case Stream::STREAM_ABORTED:
       // Handle this as connection reset.
-      NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED,
-                                       net::ERR_CONNECTION_RESET));
-      return false;
+      return net::ERR_CONNECTION_RESET;
   }
   NOTREACHED();
-  return false;
+  return net::ERR_FAILED;
 }
 
 bool StreamURLRequestJob::GetMimeType(std::string* mime_type) const {
   if (!response_info_)
     return false;
 
   // TODO(zork): Support registered MIME types if needed.
   return response_info_->headers->GetMimeType(mime_type);
 }
 
@@ skipping 38 matching lines @@
     NotifyFailure(net::ERR_METHOD_NOT_SUPPORTED);
     return;
   }
 
   HeadersCompleted(net::HTTP_OK);
 }
 
 void StreamURLRequestJob::NotifyFailure(int error_code) {
   request_failed_ = true;
 
-  // If we already return the headers on success, we can't change the headers
-  // now. Instead, we just error out.
-  if (headers_set_) {
-    NotifyDone(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, error_code));
-    return;
-  }
+  // This method can only be called before headers are set.
+  DCHECK(!headers_set_);
 
   // TODO(zork): Share these with BlobURLRequestJob.
   net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR;
   switch (error_code) {
     case net::ERR_ACCESS_DENIED:
       status_code = net::HTTP_FORBIDDEN;
       break;
     case net::ERR_FILE_NOT_FOUND:
       status_code = net::HTTP_NOT_FOUND;
       break;
@@ skipping 33 matching lines @@
 }
 
 void StreamURLRequestJob::ClearStream() {
   if (stream_.get()) {
     stream_->RemoveReadObserver(this);
     stream_ = NULL;
   }
 }
 
 }  // namespace content