URLRequestJob: change ReadRawData contract

net/url_request/url_request_file_job.cc

 // Copyright (c) 2012 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.
 
 // For loading files, we make use of overlapped i/o to ensure that reading from
 // the filesystem (e.g., a network filesystem) does not block the calling
 // thread.  An alternative approach would be to use a background thread or pool
 // of threads, but it seems better to leverage the operating system's ability
 // to do background file reads for us.
 //
@@ skipping 45 matching lines @@
 URLRequestFileJob::URLRequestFileJob(
     URLRequest* request,
     NetworkDelegate* network_delegate,
     const base::FilePath& file_path,
     const scoped_refptr<base::TaskRunner>& file_task_runner)
     : URLRequestJob(request, network_delegate),
       file_path_(file_path),
       stream_(new FileStream(file_task_runner)),
       file_task_runner_(file_task_runner),
       remaining_bytes_(0),
+      range_parse_result_(net::OK),

[mmenke, 2015/10/27 19:41:07]

net:: not needed.

[xunjieli, 2015/10/27 21:24:07]

Done.

       weak_ptr_factory_(this) {}
 
 void URLRequestFileJob::Start() {
   FileMetaInfo* meta_info = new FileMetaInfo();
   file_task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::Bind(&URLRequestFileJob::FetchMetaInfo, file_path_,
                  base::Unretained(meta_info)),
       base::Bind(&URLRequestFileJob::DidFetchMetaInfo,
                  weak_ptr_factory_.GetWeakPtr(),
                  base::Owned(meta_info)));
 }
 
 void URLRequestFileJob::Kill() {
   stream_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   URLRequestJob::Kill();
 }
 
-bool URLRequestFileJob::ReadRawData(IOBuffer* dest,
-                                    int dest_size,
-                                    int* bytes_read) {
+int URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size) {
   DCHECK_NE(dest_size, 0);
-  DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
   if (remaining_bytes_ < dest_size)
-    dest_size = static_cast<int>(remaining_bytes_);
+    dest_size = remaining_bytes_;
 
   // If we should copy zero bytes because |remaining_bytes_| is zero, short
   // circuit here.
-  if (!dest_size) {
-    *bytes_read = 0;
-    return true;
-  }
+  if (!dest_size)
+    return 0;
 
   int rv = stream_->Read(dest,
                          dest_size,
                          base::Bind(&URLRequestFileJob::DidRead,
                                     weak_ptr_factory_.GetWeakPtr(),
                                     make_scoped_refptr(dest)));
   if (rv >= 0) {
-    // Data is immediately available.
-    *bytes_read = rv;
     remaining_bytes_ -= rv;
     DCHECK_GE(remaining_bytes_, 0);
-    return true;
   }
 
-  // Otherwise, a read error occured.  We may just need to wait...
-  if (rv == ERR_IO_PENDING) {
-    SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
-  } else {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
-  }
-  return false;
+  return rv;
 }
 
 bool URLRequestFileJob::IsRedirectResponse(GURL* location,
                                            int* http_status_code) {
   if (meta_info_.is_directory) {
     // This happens when we discovered the file is a directory, so needs a
     // slash at the end of the path.
     std::string new_path = request_->url().path();
     new_path.push_back('/');
     GURL::Replacements replacements;
@@ skipping 35 matching lines @@
 
 bool URLRequestFileJob::GetMimeType(std::string* mime_type) const {
   DCHECK(request_);
   if (meta_info_.mime_type_result) {
     *mime_type = meta_info_.mime_type;
     return true;
   }
   return false;
 }
 
+// Extracts headers that this job cares about from the supplied request headers.
+// Currently this job only cares about the Range header. Note that validation is
+// deferred to DidOpen, because NotifyStartError is not legal to call since
+// the job has not started.

[mmenke, 2015/10/27 19:41:08]

Method level documentation about what a method does generally goes in the
header, docs about how it does it generally goes just above the relevant code
inside the method body.

Suggest merging the part of the comment about NotifyStartError with the comment
below the GetHeader line, and the comment that it only cares about range headers
just above that line.

[xunjieli, 2015/10/27 21:24:07]

Done.

 void URLRequestFileJob::SetExtraRequestHeaders(
     const HttpRequestHeaders& headers) {
   std::string range_header;
   if (headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
-    // We only care about "Range" header here.
+    // We only care about "Range" header here. This method stashes the value for
+    // later use in DidOpen(), which is responsible for some of the range
+    // validation as well.
     std::vector<HttpByteRange> ranges;
     if (HttpUtil::ParseRangeHeader(range_header, &ranges)) {
       if (ranges.size() == 1) {
         byte_range_ = ranges[0];
       } else {
         // We don't support multiple range requests in one single URL request,
         // because we need to do multipart encoding here.
         // TODO(hclam): decide whether we want to support multiple range
         // requests.
-        NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                    ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+        range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
       }
     }
   }
 }
 
 void URLRequestFileJob::OnSeekComplete(int64 result) {
 }
 
 void URLRequestFileJob::OnReadComplete(IOBuffer* buf, int result) {
 }
@@ skipping 40 matching lines @@
               base::File::FLAG_ASYNC;
   int rv = stream_->Open(file_path_, flags,
                          base::Bind(&URLRequestFileJob::DidOpen,
                                     weak_ptr_factory_.GetWeakPtr()));
   if (rv != ERR_IO_PENDING)
     DidOpen(rv);
 }
 
 void URLRequestFileJob::DidOpen(int result) {
   if (result != OK) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, result));
+    return;
+  }
+
+  if (range_parse_result_ != net::OK) {
+    NotifyStartError(
+        URLRequestStatus(URLRequestStatus::FAILED, range_parse_result_));
     return;
   }
 
   if (!byte_range_.ComputeBounds(meta_info_.file_size)) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-               ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED,
+                                      net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
   remaining_bytes_ = byte_range_.last_byte_position() -
                      byte_range_.first_byte_position() + 1;
   DCHECK_GE(remaining_bytes_, 0);
 
   if (remaining_bytes_ > 0 && byte_range_.first_byte_position() != 0) {
     int rv = stream_->Seek(byte_range_.first_byte_position(),
                            base::Bind(&URLRequestFileJob::DidSeek,
                                       weak_ptr_factory_.GetWeakPtr()));
     if (rv != ERR_IO_PENDING) {
       // stream_->Seek() failed, so pass an intentionally erroneous value
       // into DidSeek().
       DidSeek(-1);
     }
   } else {
     // We didn't need to call stream_->Seek() at all, so we pass to DidSeek()
     // the value that would mean seek success. This way we skip the code
     // handling seek failure.
     DidSeek(byte_range_.first_byte_position());
   }
 }
 
 void URLRequestFileJob::DidSeek(int64 result) {
   OnSeekComplete(result);
   if (result != byte_range_.first_byte_position()) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+    NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED,
+                                      ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
   set_expected_content_size(remaining_bytes_);
   NotifyHeadersComplete();
 }
 
 void URLRequestFileJob::DidRead(scoped_refptr<IOBuffer> buf, int result) {
-  if (result > 0) {
-    SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
+  if (result >= 0) {
     remaining_bytes_ -= result;
     DCHECK_GE(remaining_bytes_, 0);
   }
 
   OnReadComplete(buf.get(), result);
   buf = NULL;
 
-  if (result == 0) {
-    NotifyDone(URLRequestStatus());
-  } else if (result < 0) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
-  }
-
-  NotifyReadComplete(result);
+  ReadRawDataComplete(result);
 }
 
 }  // namespace net