Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2176)

Unified Diff: content/browser/streams/stream_url_request_job.cc

Issue 1467603002: URLRequestJob: change ReadRawData contract (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: add done_reading_called_ to MockNetworkTransaction Created 5 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « content/browser/streams/stream_url_request_job.h ('k') | content/browser/webui/url_data_manager_backend.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: content/browser/streams/stream_url_request_job.cc
diff --git a/content/browser/streams/stream_url_request_job.cc b/content/browser/streams/stream_url_request_job.cc
index 0392d108fbf935868d7b225d54228a8bbeaae822..e26fe35a41a66b90eaae5ee414d5b06a1d3761f6 100644
--- a/content/browser/streams/stream_url_request_job.cc
+++ b/content/browser/streams/stream_url_request_job.cc
@@ -40,8 +40,6 @@ StreamURLRequestJob::~StreamURLRequestJob() {
}
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())
@@ -50,24 +48,22 @@ void StreamURLRequestJob::OnDataAvailable(Stream* stream) {
// 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;
}
@@ -76,8 +72,9 @@ void StreamURLRequestJob::OnDataAvailable(Stream* stream) {
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.
@@ -94,43 +91,40 @@ void StreamURLRequestJob::Kill() {
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 {
@@ -189,13 +183,8 @@ void StreamURLRequestJob::DidStart() {
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;
« no previous file with comments | « content/browser/streams/stream_url_request_job.h ('k') | content/browser/webui/url_data_manager_backend.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698