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

Unified Diff: net/url_request/url_request.cc

Issue 2262653003: Make URLRequest::Read to return net errors or bytes read instead of a bool (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: more fixes Created 4 years, 4 months 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
Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index eac30aeb4157f9f54ea97f57c3ce1c6d2a91fbb6..627b38d61ed735d61099ded093fcc27418c49af6 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -159,6 +159,25 @@ void URLRequest::Delegate::OnSSLCertificateError(URLRequest* request,
request->Cancel();
}
+void URLRequest::Delegate::NotifyOnResponseStarted(URLRequest* request,
+ int net_error) {
+ DCHECK(request);
+
+ implemented_ = true;
+
+ OnResponseStarted(request, net_error);
+ if (!implemented_)
mmenke 2016/08/26 19:36:48 The issue with the failures is that the OnResponse
maksims (do not use this acc) 2016/08/29 12:30:35 Oh, thanks! Good to know this trick!
+ OnResponseStarted(request);
+}
+
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request,
+ int net_error) {
+ NOTIMPLEMENTED();
+ implemented_ = false;
+}
+
+void URLRequest::Delegate::OnResponseStarted(URLRequest* request) {}
+
///////////////////////////////////////////////////////////////////////////////
// URLRequest
@@ -496,6 +515,9 @@ void URLRequest::set_delegate(Delegate* delegate) {
void URLRequest::Start() {
DCHECK(delegate_);
+ if (!status_.is_success())
+ return;
+
// TODO(pkasting): Remove ScopedTracker below once crbug.com/456327 is fixed.
tracked_objects::ScopedTracker tracking_profile(
FROM_HERE_WITH_EXPLICIT_FUNCTION("456327 URLRequest::Start"));
@@ -555,6 +577,7 @@ URLRequest::URLRequest(const GURL& url,
first_party_url_policy_(NEVER_CHANGE_FIRST_PARTY_URL),
load_flags_(LOAD_NORMAL),
delegate_(delegate),
+ status_(URLRequestStatus::FromError(OK)),
is_pending_(false),
is_redirecting_(false),
redirect_limit_(kMaxRedirects),
@@ -672,12 +695,12 @@ void URLRequest::RestartWithJob(URLRequestJob *job) {
StartJob(job);
}
-void URLRequest::Cancel() {
- DoCancel(ERR_ABORTED, SSLInfo());
+int URLRequest::Cancel() {
+ return DoCancel(ERR_ABORTED, SSLInfo());
}
-void URLRequest::CancelWithError(int error) {
- DoCancel(error, SSLInfo());
+int URLRequest::CancelWithError(int error) {
+ return DoCancel(error, SSLInfo());
}
void URLRequest::CancelWithSSLError(int error, const SSLInfo& ssl_info) {
@@ -689,7 +712,7 @@ void URLRequest::CancelWithSSLError(int error, const SSLInfo& ssl_info) {
DoCancel(error, ssl_info);
}
-void URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
+int URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
DCHECK(error < 0);
// If cancelled while calling a delegate, clear delegate info.
if (calling_delegate_) {
@@ -722,8 +745,46 @@ void URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
// The Job will call our NotifyDone method asynchronously. This is done so
// that the Delegate implementation can call Cancel without having to worry
// about being called recursively.
+ return status_.error();
+}
+
+int URLRequest::Read(IOBuffer* dest, int dest_size) {
+ DCHECK(job_.get());
+
+ // If this is the first read, end the delegate call that may have started in
+ // OnResponseStarted.
+ OnCallToDelegateComplete();
+
+ // Once the request fails or is cancelled, read will just return 0 bytes
+ // to indicate end of stream.
+ if (!status_.is_success()) {
mmenke 2016/08/26 19:36:48 Can this happen, if job_->is_done() is false? (Al
maksims (do not use this acc) 2016/08/29 12:30:35 Yes, if URLRequestJob::NotifyStartError() is calle
+ return status_.error();
+ }
+
+ // This handles a cancel that happens while paused.
+ // TODO(ahendrickson): DCHECK() that it is not done after
+ // http://crbug.com/115705 is fixed.
+ if (job_->is_done())
+ return status_.error();
+
+ if (dest_size == 0) {
+ // Caller is not too bright. I guess we've done what they asked.
+ return OK;
+ }
+
+ int rv = job_->Read(dest, dest_size);
+ if (rv == ERR_IO_PENDING)
+ set_status(URLRequestStatus::FromError(ERR_IO_PENDING));
+
+ // If rv is not 0 or actual bytes read, the status cannot be success.
+ DCHECK(!(rv < 0) || status_.status() != URLRequestStatus::SUCCESS);
mmenke 2016/08/26 19:36:48 Think rv >= 0 is easier to read, for the first che
maksims (do not use this acc) 2016/08/29 12:30:35 Done.
+
+ if (rv == 0 && status_.is_success())
+ NotifyRequestCompleted();
+ return rv;
}
+// Deprecated.
bool URLRequest::Read(IOBuffer* dest, int dest_size, int* bytes_read) {
DCHECK(job_.get());
DCHECK(bytes_read);
mmenke 2016/08/26 19:36:48 Isn't everything below this the same as: int resu
maksims (do not use this acc) 2016/08/29 12:30:35 Right! Thanks!
@@ -826,7 +887,7 @@ void URLRequest::NotifyResponseStarted(const URLRequestStatus& status) {
// completion event and receive a NotifyResponseStarted() later.
if (!has_notified_completion_ && status_.is_success()) {
if (network_delegate_)
- network_delegate_->NotifyResponseStarted(this);
+ network_delegate_->NotifyResponseStarted(this, net_error);
}
// Notify in case the entire URL Request has been finished.
@@ -834,7 +895,7 @@ void URLRequest::NotifyResponseStarted(const URLRequestStatus& status) {
NotifyRequestCompleted();
OnCallToDelegate();
- delegate_->OnResponseStarted(this);
+ delegate_->NotifyOnResponseStarted(this, net_error);
// Nothing may appear below this line as OnResponseStarted may delete
// |this|.
}
@@ -1140,6 +1201,12 @@ void URLRequest::NotifyReadCompleted(int bytes_read) {
if (bytes_read <= 0)
NotifyRequestCompleted();
+ // When URLRequestJob notices there was an error in URLRequest's |status_|,
+ // it calls this method with |bytes_read| set to -1. Set it to a real error
+ // here.
+ if (bytes_read == -1)
+ bytes_read = status_.error();
+
// Notify NetworkChangeNotifier that we just received network data.
// This is to identify cases where the NetworkChangeNotifier thinks we
// are off-line but we are still receiving network data (crbug.com/124069),
@@ -1184,7 +1251,8 @@ void URLRequest::NotifyRequestCompleted() {
is_redirecting_ = false;
has_notified_completion_ = true;
if (network_delegate_)
- network_delegate_->NotifyCompleted(this, job_.get() != NULL);
+ network_delegate_->NotifyCompleted(this, job_.get() != NULL,
+ status_.error());
}
void URLRequest::OnCallToDelegate() {

Powered by Google App Engine
This is Rietveld 408576698