JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 152ec3f3d71fd62294f9d379d4eb677affd24ce8..6c2a9018b942e82433c2867d73f5befb4ef9cf8b 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -35,6 +35,7 @@
 #include "net/http/http_request_info.h"
 #include "net/http/http_server_properties.h"
 #include "net/http/http_stream_factory.h"
+#include "net/http/http_stream_factory_impl_job_controller.h"
 #include "net/http/http_stream_factory_impl_request.h"
 #include "net/log/net_log.h"
 #include "net/quic/quic_http_stream.h"
@@ -154,7 +155,7 @@ std::unique_ptr<base::Value> NetLogHttpStreamProtoCallback(
   return std::move(dict);
 }
 
-HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
+HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
@@ -163,7 +164,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
                                 HostPortPair destination,
                                 GURL origin_url,
                                 NetLog* net_log)
-    : Job(stream_factory,
+    : Job(job_controller,
           session,
           request_info,
           priority,
@@ -174,7 +175,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
           AlternativeService(),
           net_log) {}
 
-HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
+HttpStreamFactoryImpl::Job::Job(JobController* job_controller,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
@@ -184,8 +185,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
                                 GURL origin_url,
                                 AlternativeService alternative_service,
                                 NetLog* net_log)
-    : request_(NULL),
-      request_info_(request_info),
+    : request_info_(request_info),
       priority_(priority),
       server_ssl_config_(server_ssl_config),
       proxy_ssl_config_(proxy_ssl_config),
@@ -193,14 +193,15 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       io_callback_(base::Bind(&Job::OnIOComplete, base::Unretained(this))),
       connection_(new ClientSocketHandle),
       session_(session),
-      stream_factory_(stream_factory),
       next_state_(STATE_NONE),
       pac_request_(NULL),
       destination_(destination),
       origin_url_(origin_url),
       alternative_service_(alternative_service),
+      job_controller_(job_controller),
       blocking_job_(NULL),
       waiting_job_(NULL),
+      orphaned_(false),
       using_ssl_(false),
       using_spdy_(false),
       using_quic_(false),
@@ -216,7 +217,6 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       other_job_status_(STATUS_RUNNING),
       stream_type_(HttpStreamRequest::BIDIRECTIONAL_STREAM),
       ptr_factory_(this) {
-  DCHECK(stream_factory);
   DCHECK(session);
   if (IsQuicAlternative()) {
     DCHECK(session_->params().enable_quic);
@@ -245,9 +245,7 @@ HttpStreamFactoryImpl::Job::~Job() {
 
 void HttpStreamFactoryImpl::Job::Start(Request* request) {
   DCHECK(request);
-  request_ = request;
-  // Saves |stream_type_|, since request is nulled when job is orphaned.
-  stream_type_ = request_->stream_type();
+  stream_type_ = request->stream_type();

[Ryan Hamilton, 2016/05/06 20:49:01]

nit: In that case, I'd just pass in the stream_type instead of the full request.
You might be able to do this in constructor, though I'm not sure about that.

[Randy Smith (Not in Mondays), 2016/05/09 21:42:09]

Agreed.  Specifically, the design flow sorta promises a lack of interaction
between Request and Job; all such interactions now go through JobController. 
What's left is minimal, but leaving any such connections behind invites future
modifications to increase those connections.  So I'd be a bit ruthless in
removing such references.

[Zhongyi Shi, 2016/05/12 07:26:23]

Done. It's actually done in a subsequent CL, but I will be happy to bring it
ahead.

   StartInternal();
 }
 
@@ -327,8 +325,9 @@ void HttpStreamFactoryImpl::Job::Resume(Job* job,
 }
 
 void HttpStreamFactoryImpl::Job::Orphan(const Request* request) {

[Randy Smith (Not in Mondays), 2016/05/09 21:42:09]

This routine no longer uses the request argument; remove it?  (See above for
context.)

[Zhongyi Shi, 2016/05/12 07:26:23]

Done.

-  DCHECK_EQ(request_, request);
-  request_ = NULL;
+  DCHECK(!orphaned_);
+  if (!IsPreconnecting())

[Ryan Hamilton, 2016/05/06 20:49:01]

Why not set this true for the preconnect case?

[Randy Smith (Not in Mondays), 2016/05/09 20:42:18]

I think to keep the old behavior--IsOrphaned() used to return false if
preconnect.

[Zhongyi Shi, 2016/05/12 07:26:23]

orphaned_ indicates whether |this| job has ever been associated with a Request
but then Orphaned. Preconnect job is never orphaned as it's never been
associated with a Request. 

+    orphaned_ = true;
   net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_ORPHANED);
   if (blocking_job_) {
     // We've been orphaned, but there's a job we're blocked on. Don't bother
@@ -336,19 +335,19 @@ void HttpStreamFactoryImpl::Job::Orphan(const Request* request) {
     DCHECK(blocking_job_->waiting_job_);
     blocking_job_->waiting_job_ = NULL;
     blocking_job_ = NULL;
-    if (stream_factory_->for_websockets_ &&
-        connection_ && connection_->socket()) {
+    if (job_controller_->for_websockets() && connection_ &&

[Randy Smith (Not in Mondays), 2016/05/09 20:42:18]

What aspects of your change changed the lifecycle such that connection_ could be
null at this point now but couldn't before?

[Randy Smith (Not in Mondays), 2016/05/09 21:42:09]

Whoops, sorry, missed that this is identical to previous code.

+        connection_->socket()) {
       connection_->socket()->Disconnect();
     }
-    stream_factory_->OnOrphanedJobComplete(this);
-  } else if (stream_factory_->for_websockets_) {
+    job_controller_->OnOrphanedJobComplete(this);
+  } else if (job_controller_->for_websockets()) {
     // We cancel this job because a WebSocketHandshakeStream can't be created
     // without a WebSocketHandshakeStreamBase::CreateHelper which is stored in
     // the Request class and isn't accessible from this job.
     if (connection_ && connection_->socket()) {
       connection_->socket()->Disconnect();
     }
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   }
 }
 
@@ -418,7 +417,7 @@ bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
 void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
   DCHECK(stream_.get());
   DCHECK(!IsPreconnecting());
-  DCHECK(!stream_factory_->for_websockets_);
+  DCHECK(!job_controller_->for_websockets());
 
   UMA_HISTOGRAM_TIMES("Net.HttpStreamFactoryJob.StreamReadyCallbackTime",
                       base::TimeTicks::Now() - job_stream_ready_start_time_);
@@ -426,12 +425,12 @@ void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
   if (IsOrphaned()) {

[Ryan Hamilton, 2016/05/06 20:49:01]

Ooo! You're going to love (or probably) hate this :>

The controller knows (or could easily know) if a job is the bound or orphaned
job. As a result, I think the job can just do:

job_controller_->MarkRequestComplete(...)
job_controller_->OnStreamReady(...)

and the controller can do the check to see if the job has been orphaned. This
will make the job simpler. (Here and elsewhere). In fact, it wouldn't surprise
me if this meant that the job no longer needed to know about being orphaned,
which would be nice!

[Randy Smith (Not in Mondays), 2016/05/09 21:42:09]

+1, if it can be done.

[Zhongyi Shi, 2016/05/12 07:26:23]

Done.

-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   } else {
-    request_->Complete(was_npn_negotiated(), protocol_negotiated(),
-                       using_spdy());
-    request_->OnStreamReady(this, server_ssl_config_, proxy_info_,
-                            stream_.release());
+    job_controller_->MarkRequestComplete(was_npn_negotiated(),
+                                         protocol_negotiated(), using_spdy());
+    job_controller_->OnStreamReady(this, server_ssl_config_, proxy_info_,
+                                   stream_.release());
   }
   // |this| may be deleted after this call.
 }
@@ -439,18 +438,17 @@ void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
 void HttpStreamFactoryImpl::Job::OnWebSocketHandshakeStreamReadyCallback() {
   DCHECK(websocket_stream_);
   DCHECK(!IsPreconnecting());
-  DCHECK(stream_factory_->for_websockets_);
+  DCHECK(job_controller_->for_websockets());
   // An orphaned WebSocket job will be closed immediately and
   // never be ready.
   DCHECK(!IsOrphaned());
 
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
-  request_->Complete(was_npn_negotiated(), protocol_negotiated(), using_spdy());
-  request_->OnWebSocketHandshakeStreamReady(this,
-                                            server_ssl_config_,
-                                            proxy_info_,
-                                            websocket_stream_.release());
+  job_controller_->MarkRequestComplete(was_npn_negotiated(),
+                                       protocol_negotiated(), using_spdy());
+  job_controller_->OnWebSocketHandshakeStreamReady(
+      this, server_ssl_config_, proxy_info_, websocket_stream_.release());
   // |this| may be deleted after this call.
 }
 
@@ -460,11 +458,11 @@ void HttpStreamFactoryImpl::Job::OnBidirectionalStreamImplReadyCallback() {
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
   if (IsOrphaned()) {
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   } else {
-    request_->Complete(was_npn_negotiated(), protocol_negotiated(),
-                       using_spdy());
-    request_->OnBidirectionalStreamImplReady(
+    job_controller_->MarkRequestComplete(was_npn_negotiated(),
+                                         protocol_negotiated(), using_spdy());
+    job_controller_->OnBidirectionalStreamImplReady(
         this, server_ssl_config_, proxy_info_,
         bidirectional_stream_impl_.release());
   }
@@ -482,26 +480,28 @@ void HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback() {
 
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
-  // TODO(jgraettinger): Notify the factory, and let that notify |request_|,
-  // rather than notifying |request_| directly.
+  // Notify the JobController, and let that notify Request and Factory.
   if (IsOrphaned()) {
     if (spdy_session) {
-      stream_factory_->OnNewSpdySessionReady(
-          spdy_session, spdy_session_direct_, server_ssl_config_, proxy_info_,
-          was_npn_negotiated(), protocol_negotiated(), using_spdy(), net_log_);
+      job_controller_->OnNewSpdySessionReady(
+          this, /*spdy_http_stream=*/nullptr,
+          /* bidirectional_stream_impl=*/nullptr, spdy_session,
+          spdy_session_direct_);
     }
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   } else {
     if (stream_type_ == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
       DCHECK(bidirectional_stream_impl_);
-      request_->OnNewSpdySessionReady(this, /*spdy_http_stream=*/nullptr,
-                                      std::move(bidirectional_stream_impl_),
-                                      spdy_session, spdy_session_direct_);
+      job_controller_->OnNewSpdySessionReady(
+          this, /*spdy_http_stream=*/nullptr,
+          std::move(bidirectional_stream_impl_), spdy_session,
+          spdy_session_direct_);
     } else {
       DCHECK(stream_);
-      request_->OnNewSpdySessionReady(this, std::move(stream_),
-                                      /** bidirectional_stream_impl=*/nullptr,
-                                      spdy_session, spdy_session_direct_);
+      job_controller_->OnNewSpdySessionReady(
+          this, std::move(stream_),
+          /* bidirectional_stream_impl=*/nullptr, spdy_session,
+          spdy_session_direct_);
     }
   }
   // |this| may be deleted after this call.
@@ -513,12 +513,12 @@ void HttpStreamFactoryImpl::Job::OnStreamFailedCallback(int result) {
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
   if (IsOrphaned()) {
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   } else {
     SSLFailureState ssl_failure_state =
         connection_ ? connection_->ssl_failure_state() : SSL_FAILURE_NONE;
-    request_->OnStreamFailed(this, result, server_ssl_config_,
-                             ssl_failure_state);
+    job_controller_->OnStreamFailed(this, result, server_ssl_config_,
+                                    ssl_failure_state);
   }
   // |this| may be deleted after this call.
 }
@@ -530,9 +530,10 @@ void HttpStreamFactoryImpl::Job::OnCertificateErrorCallback(
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
   if (IsOrphaned())
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   else
-    request_->OnCertificateError(this, result, server_ssl_config_, ssl_info);
+    job_controller_->OnCertificateError(this, result, server_ssl_config_,
+                                        ssl_info);
   // |this| may be deleted after this call.
 }
 
@@ -541,10 +542,10 @@ void HttpStreamFactoryImpl::Job::OnNeedsProxyAuthCallback(
     HttpAuthController* auth_controller) {
   DCHECK(!IsPreconnecting());
   if (IsOrphaned())
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   else
-    request_->OnNeedsProxyAuth(
-        this, response, server_ssl_config_, proxy_info_, auth_controller);
+    job_controller_->OnNeedsProxyAuth(this, response, server_ssl_config_,
+                                      proxy_info_, auth_controller);
   // |this| may be deleted after this call.
 }
 
@@ -552,9 +553,9 @@ void HttpStreamFactoryImpl::Job::OnNeedsClientAuthCallback(
     SSLCertRequestInfo* cert_info) {
   DCHECK(!IsPreconnecting());
   if (IsOrphaned())
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   else
-    request_->OnNeedsClientAuth(this, server_ssl_config_, cert_info);
+    job_controller_->OnNeedsClientAuth(this, server_ssl_config_, cert_info);
   // |this| may be deleted after this call.
 }
 
@@ -563,26 +564,19 @@ void HttpStreamFactoryImpl::Job::OnHttpsProxyTunnelResponseCallback(
     HttpStream* stream) {
   DCHECK(!IsPreconnecting());
   if (IsOrphaned())
-    stream_factory_->OnOrphanedJobComplete(this);
+    job_controller_->OnOrphanedJobComplete(this);
   else
-    request_->OnHttpsProxyTunnelResponse(
+    job_controller_->OnHttpsProxyTunnelResponse(
         this, response_info, server_ssl_config_, proxy_info_, stream);
   // |this| may be deleted after this call.
 }
 
 void HttpStreamFactoryImpl::Job::OnPreconnectsComplete() {
-  DCHECK(!request_);
   if (new_spdy_session_.get()) {
-    stream_factory_->OnNewSpdySessionReady(new_spdy_session_,
-                                           spdy_session_direct_,
-                                           server_ssl_config_,
-                                           proxy_info_,
-                                           was_npn_negotiated(),
-                                           protocol_negotiated(),
-                                           using_spdy(),
-                                           net_log_);
-  }
-  stream_factory_->OnPreconnectsComplete(this);
+    job_controller_->OnNewSpdySessionReady(
+        this, nullptr, nullptr, new_spdy_session_, spdy_session_direct_);
+  }
+  job_controller_->OnPreconnectsComplete(this);
   // |this| may be deleted after this call.
 }
 
@@ -692,7 +686,7 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::Bind(&Job::OnNewSpdySessionReadyCallback,
                                   ptr_factory_.GetWeakPtr()));
-      } else if (stream_factory_->for_websockets_) {
+      } else if (job_controller_->for_websockets()) {
         DCHECK(websocket_stream_);
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::Bind(&Job::OnWebSocketHandshakeStreamReadyCallback,
@@ -808,12 +802,15 @@ int HttpStreamFactoryImpl::Job::DoStart() {
   net_log_.BeginEvent(
       NetLog::TYPE_HTTP_STREAM_JOB,
       base::Bind(&NetLogHttpStreamJobCallback,
-                 request_ ? request_->net_log().source() : NetLog::Source(),
+                 (!IsPreconnecting() && !orphaned_)
+                     ? job_controller_->GetNetLogFromRequest().source()
+                     : NetLog::Source(),

[Randy Smith (Not in Mondays), 2016/05/09 21:42:09]

Hmmm.  Parallel to Ryan's suggestions above, I'd be tempted to push the source
choice up into the JobController, and have it figure out whether it wants this
logged or not.  

It's a behavior change, but I could imagine at some point we'd want to log these
even if they're preconnecting or orphaned.

[Zhongyi Shi, 2016/05/12 07:26:23]

Done.

                  &request_info_.url, &origin_url_, &alternative_service_,
                  priority_));
-  if (request_) {
-    request_->net_log().AddEvent(NetLog::TYPE_HTTP_STREAM_REQUEST_STARTED_JOB,
-                                 net_log_.source().ToEventParametersCallback());
+  if (!IsPreconnecting() && !orphaned_) {
+    job_controller_->GetNetLogFromRequest().AddEvent(
+        NetLog::TYPE_HTTP_STREAM_REQUEST_STARTED_JOB,
+        net_log_.source().ToEventParametersCallback());
   }
 
   // Don't connect to restricted ports.
@@ -1056,9 +1053,10 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
       return OK;
     }
   }
-  if (request_ && !request_->HasSpdySessionKey() && using_ssl_) {
-    // Update the spdy session key for the request that launched this job.
-    request_->SetSpdySessionKey(spdy_session_key);
+  if (!IsPreconnecting() && !orphaned_ && using_ssl_) {
+    // Ask |job_controller_| to update the spdy session key for the request
+    // that launched this job.
+    job_controller_->SetSpdySessionKey(spdy_session_key);
   }
 
   // OK, there's no available SPDY session. Let |waiting_job_| resume if it's
@@ -1084,7 +1082,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   }
 
   if (IsPreconnecting()) {
-    DCHECK(!stream_factory_->for_websockets_);
+    DCHECK(!job_controller_->for_websockets());
     return PreconnectSocketsForHttpRequest(
         GetSocketGroup(), destination_, request_info_.extra_headers,
         request_info_.load_flags, priority_, session_, proxy_info_, expect_spdy,
@@ -1099,7 +1097,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
           ? base::Bind(&Job::OnHostResolution, session_->spdy_session_pool(),
                        spdy_session_key, origin_url_)
           : OnHostResolutionCallback();
-  if (stream_factory_->for_websockets_) {
+  if (job_controller_->for_websockets()) {
     // TODO(ricea): Re-enable NPN when WebSockets over SPDY is supported.
     SSLConfig websocket_server_ssl_config = server_ssl_config_;
     websocket_server_ssl_config.alpn_protos.clear();
@@ -1317,7 +1315,7 @@ int HttpStreamFactoryImpl::Job::SetSpdyHttpStreamOrBidirectionalStreamImpl(
     bool direct) {
   // TODO(ricea): Restore the code for WebSockets over SPDY once it's
   // implemented.
-  if (stream_factory_->for_websockets_)
+  if (job_controller_->for_websockets())
     return ERR_NOT_IMPLEMENTED;
   if (stream_type_ == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
     bidirectional_stream_impl_.reset(new BidirectionalStreamSpdyImpl(session));
@@ -1362,11 +1360,11 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
                        (request_info_.url.SchemeIs("http") ||
                         request_info_.url.SchemeIs("ftp"));
-    if (stream_factory_->for_websockets_) {
-      DCHECK(request_);
-      DCHECK(request_->websocket_handshake_stream_create_helper());
+    if (job_controller_->for_websockets()) {
+      DCHECK(!IsPreconnecting() && !orphaned_);
+      DCHECK(job_controller_->websocket_handshake_stream_create_helper());
       websocket_stream_.reset(
-          request_->websocket_handshake_stream_create_helper()
+          job_controller_->websocket_handshake_stream_create_helper()
               ->CreateBasicStream(std::move(connection_), using_proxy));
     } else {
       stream_.reset(new HttpBasicStream(connection_.release(), using_proxy));
@@ -1488,8 +1486,9 @@ void HttpStreamFactoryImpl::Job::ReturnToStateInitConnection(
     connection_->socket()->Disconnect();
   connection_->Reset();
 
-  if (request_)
-    request_->RemoveRequestFromSpdySessionRequestMap();
+  if (!IsPreconnecting() && !orphaned_) {
+    job_controller_->RemoveRequestFromSpdySessionRequestMap();
+  }
 
   next_state_ = STATE_INIT_CONNECTION;
 }
@@ -1631,8 +1630,9 @@ int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
     if (connection_->socket())
       connection_->socket()->Disconnect();
     connection_->Reset();
-    if (request_)
-      request_->RemoveRequestFromSpdySessionRequestMap();
+    if (!IsPreconnecting() && !orphaned_) {
+      job_controller_->RemoveRequestFromSpdySessionRequestMap();
+    }
     next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
   } else {
     // If ReconsiderProxyAfterError() failed synchronously, it means
@@ -1688,10 +1688,6 @@ bool HttpStreamFactoryImpl::Job::IsPreconnecting() const {
   return num_streams_ > 0;
 }
 
-bool HttpStreamFactoryImpl::Job::IsOrphaned() const {
-  return !IsPreconnecting() && !request_;
-}
-
 void HttpStreamFactoryImpl::Job::ReportJobSucceededForRequest() {
   if (using_existing_quic_session_) {
     // If an existing session was used, then no TCP connection was
@@ -1802,15 +1798,16 @@ HttpStreamFactoryImpl::Job::GetSocketGroup() const {
 // connection attempts from the proper place.
 void HttpStreamFactoryImpl::Job::
     MaybeCopyConnectionAttemptsFromSocketOrHandle() {
-  if (IsOrphaned() || !connection_)
+  if (IsPreconnecting() || IsOrphaned() || !connection_)
     return;
 
   if (connection_->socket()) {
     ConnectionAttempts socket_attempts;
     connection_->socket()->GetConnectionAttempts(&socket_attempts);
-    request_->AddConnectionAttempts(socket_attempts);
+    job_controller_->AddConnectionAttemptsToRequest(socket_attempts);
   } else {
-    request_->AddConnectionAttempts(connection_->connection_attempts());
+    job_controller_->AddConnectionAttemptsToRequest(
+        connection_->connection_attempts());
   }
 }