Make URLRequestJob::SetStatus private.

net/url_request/url_request.cc

Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index 4d1a871d39eec390bfaee27f446f4f34ef510ddb..4c4bcfe7f6321e5f63bfe90f914defdc27c5e164 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -658,10 +658,14 @@ void URLRequest::StartJob(URLRequestJob* job) {
     }
   }
 
+  // Mark the request as pending, as Start() always completes asynchronously.
+  // Status is generally set by URLRequestJob itself, but Start() calls
+  // directly into the URLRequestJob subclass, so URLRequestJob can't set it
+  // here.
+  // TODO(mmenke):  Make the URLRequest manage its own status.
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
+
   // Don't allow errors to be sent from within Start().

[Randy Smith (Not in Mondays), 2016/01/11 02:27:48]

Do you understand this comment?  It confuses me--I don't see any code here
that's relevant to errors coming from within start, other than the lack of a
return value, which suggests to me that the comment should be in the URLRJ
header file.  Maybe "Start isn't allowed to return errors synchronously" or
removing the comment?

[mmenke, 2016/01/11 06:17:11]

I merged it with the above comment - yea, I think the point is that a result
isn't returned synchronously.  I left the comment alone originally just because
it's a behavior I want to change eventually, but agree it's good to have
reasonable comments in the meantime.

-  // TODO(brettw) this may cause NotifyDone to be sent synchronously,
-  // we probably don't want this: they should be sent asynchronously so
-  // the caller does not get reentered.

[mmenke, 2016/01/07 15:54:55]

This comment is rather outdated - NotifyDone always posts an async task, now.

[Randy Smith (Not in Mondays), 2016/01/11 02:27:48]

Acknowledged.

   job_->Start();
 }
 
@@ -855,9 +859,10 @@ void URLRequest::NotifyResponseStarted() {
 }
 
 void URLRequest::FollowDeferredRedirect() {
-  CHECK(job_.get());
-  CHECK(status_.is_success());
+  DCHECK(job_.get());
+  DCHECK(status_.is_success());
 
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);

[Randy Smith (Not in Mondays), 2016/01/11 02:27:48]

I'm struggling with my conscience as to whether this review requires me to grok
the full URLRequest state machine to verify these assignments.  Can you give me
a sense as to the logic you used in choosing them so that I can decide how deep
I have to dig to validate them?  

(Obviously, I'd love to have the state machine actually documented somewhere,
and one really nice thing about this CL is that we're moving towards a place
where that might be done.  But I don't think we're quite there yet?)

[mmenke, 2016/01/11 06:17:11]

So, we don't really distinguish well between what ERR_IO_PENDING means and what
URLRequestStatus::SUCCESS means.  In this CL, I've attempt to modify them so
that they're at least consistent:

* From request Start until URLRequestJob::NotifyHeadersComplete (Or
NotifyStartError), status is ERR_IO_PENDING.  We change it before we notify
anything (Including both delegates) of headers being received / a redirect. 
This is basically old behavior (It was just being set to OK before the methods
are invoked, instead of after).
* Any call that will result in NotifyHeadersComplete / NotifyStartError being
called again will set it to ERR_IO_PENDING again.  We weren't doing this before,
which was bizarre and made the state pretty unpredictable.
* URLRequest::Read sets it to ERR_IO_PENDING again, and it's only set to OK once
we have (post-filter) data to send to the URLRequest.

Long term, I'd really like to get rid of the distinction as a queriable state
(And get rid of URLRequestStatus as well).

   job_->FollowDeferredRedirect();
 }
 
@@ -865,6 +870,7 @@ void URLRequest::SetAuth(const AuthCredentials& credentials) {
   DCHECK(job_.get());
   DCHECK(job_->NeedsAuth());
 
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
   job_->SetAuth(credentials);
 }
 
@@ -872,6 +878,7 @@ void URLRequest::CancelAuth() {
   DCHECK(job_.get());
   DCHECK(job_->NeedsAuth());
 
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
   job_->CancelAuth();
 }
 
@@ -879,12 +886,14 @@ void URLRequest::ContinueWithCertificate(X509Certificate* client_cert,
                                          SSLPrivateKey* client_private_key) {
   DCHECK(job_.get());
 
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
   job_->ContinueWithCertificate(client_cert, client_private_key);
 }
 
 void URLRequest::ContinueDespiteLastError() {
   DCHECK(job_.get());
 
+  status_ = URLRequestStatus::FromError(ERR_IO_PENDING);
   job_->ContinueDespiteLastError();
 }
 
@@ -1111,11 +1120,13 @@ void URLRequest::NotifyAuthRequiredComplete(
 
 void URLRequest::NotifyCertificateRequested(
     SSLCertRequestInfo* cert_request_info) {
+  status_ = URLRequestStatus();
   delegate_->OnCertificateRequested(this, cert_request_info);
 }
 
 void URLRequest::NotifySSLCertificateError(const SSLInfo& ssl_info,
                                            bool fatal) {
+  status_ = URLRequestStatus();
   delegate_->OnSSLCertificateError(this, ssl_info, fatal);
 }