JobController3: Move MarkAlternativeServiceBroken to job controller

net/http/http_stream_factory_impl_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.
 
 #include "net/http/http_stream_factory_impl_job.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 188 matching lines @@
       using_spdy_(false),
       using_quic_(false),
       quic_request_(session_->quic_stream_factory()),
       using_existing_quic_session_(false),
       spdy_certificate_error_(OK),
       establishing_tunnel_(false),
       was_npn_negotiated_(false),
       negotiated_protocol_(kProtoUnknown),
       num_streams_(0),
       spdy_session_direct_(false),
       job_status_(STATUS_RUNNING),

[tbansal1, 2016/09/13 05:06:45]

It seems that job_status_, other_job_status_ and
other_job_alternative_proxy_server_ can all be removed?

[Zhongyi Shi, 2016/09/13 22:52:37]

Done.

       other_job_status_(STATUS_RUNNING),
       stream_type_(HttpStreamRequest::BIDIRECTIONAL_STREAM),
       ptr_factory_(this) {
   DCHECK(session);
   // The job can't have alternative service and alternative proxy server set at
   // the same time since alternative services are used for requests that are
   // fetched directly, while the alternative proxy server is used for requests
   // that should be fetched using proxy.
   DCHECK(alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL ||
          !alternative_proxy_server_.is_valid());
@@ skipping 354 matching lines @@
           static_cast<ProxyClientSocket*>(connection_->socket());
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&Job::OnHttpsProxyTunnelResponseCallback,
                                 ptr_factory_.GetWeakPtr(),
                                 *proxy_socket->GetConnectResponseInfo(),
                                 proxy_socket->CreateConnectResponseStream()));
       return ERR_IO_PENDING;
     }
 
     case OK:
-      job_status_ = STATUS_SUCCEEDED;
-      MaybeMarkAlternativeServiceBroken();
       next_state_ = STATE_DONE;
       if (new_spdy_session_.get()) {
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::Bind(&Job::OnNewSpdySessionReadyCallback,
                                   ptr_factory_.GetWeakPtr()));
       } else if (delegate_->for_websockets()) {
         DCHECK(websocket_stream_);
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE, base::Bind(&Job::OnWebSocketHandshakeStreamReadyCallback,
                                   ptr_factory_.GetWeakPtr()));
@@ skipping 10 matching lines @@
         }
       } else {
         DCHECK(stream_.get());
         base::ThreadTaskRunnerHandle::Get()->PostTask(
             FROM_HERE,
             base::Bind(&Job::OnStreamReadyCallback, ptr_factory_.GetWeakPtr()));
       }
       return ERR_IO_PENDING;
 
     default:
-      if (job_status_ != STATUS_BROKEN) {
-        DCHECK_EQ(STATUS_RUNNING, job_status_);
-        job_status_ = STATUS_FAILED;
-        MaybeMarkAlternativeServiceBroken();
+      // Notify job controlelr that alternative proxy server is broken.
+      if (alternative_proxy_server_.is_valid()) {
+        delegate_->OnAlternativeProxyServerBroken(this);

[Ryan Hamilton, 2016/09/13 04:17:04]

Instead of requiring the job to explicitly notify the delegate of broken-ness,
it seems like it would be cleaner to have the controller figure this out via
OnStreamFailed (called from OnStreamFailedCallback()). Would that work?

       }
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE, base::Bind(&Job::OnStreamFailedCallback,
                                 ptr_factory_.GetWeakPtr(), result));
       return ERR_IO_PENDING;
   }
 }
 
 int HttpStreamFactoryImpl::Job::DoLoop(int result) {
   DCHECK_NE(next_state_, STATE_NONE);
@@ skipping 428 matching lines @@
     connection_.reset(connection_->release_pending_http_proxy_connection());
     return result;
   }
 
   if (proxy_info_.is_quic() && using_quic_ && result < 0) {
     using_quic_ = false;
     return ReconsiderProxyAfterError(result);
   }
 
   if (IsSpdyAlternative() && !using_spdy_) {
-    job_status_ = STATUS_BROKEN;
-    MaybeMarkAlternativeServiceBroken();
+    if (alternative_proxy_server_.is_valid()) {
+      // |this| connected to the alternative proxy server.
+      delegate_->OnAlternativeProxyServerBroken(this);
+    } else {
+      delegate_->OnAlternativeServiceBroken(this);
+    }
     return ERR_NPN_NEGOTIATION_FAILED;
   }
 
   if (!ssl_started && result < 0 &&
       (IsSpdyAlternative() || IsQuicAlternative())) {
-    job_status_ = STATUS_BROKEN;
-    MaybeMarkAlternativeServiceBroken();
+    if (alternative_proxy_server_.is_valid()) {
+      // |this| connected to the alternative proxy server.
+      delegate_->OnAlternativeProxyServerBroken(this);
+    } else {
+      delegate_->OnAlternativeServiceBroken(this);
+    }
     return result;
   }
 
   if (using_quic_) {
     if (result < 0) {
-      job_status_ = STATUS_BROKEN;
-      MaybeMarkAlternativeServiceBroken();
+      if (alternative_proxy_server_.is_valid()) {
+        // |this| connected to the alternative proxy server.
+        delegate_->OnAlternativeProxyServerBroken(this);
+      } else {
+        delegate_->OnAlternativeServiceBroken(this);
+      }
       return result;
     }
     if (stream_type_ == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
       bidirectional_stream_impl_ =
           quic_request_.CreateBidirectionalStreamImpl();
       if (!bidirectional_stream_impl_) {
         // Quic session is closed before stream can be created.
         return ERR_CONNECTION_CLOSED;
       }
     } else {
@@ skipping 419 matching lines @@
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE);
   } else if (IsSpdyAlternative() || IsQuicAlternative()) {
     // This Job was the alternative Job, and hence won the race.
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE);
   } else {
     // This Job was the normal Job, and hence the alternative Job lost the race.
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE);
   }
 }
 
-void HttpStreamFactoryImpl::Job::MarkOtherJobComplete(const Job& job) {
-  DCHECK_EQ(STATUS_RUNNING, other_job_status_);
-  DCHECK(!other_job_alternative_proxy_server_.is_valid());
-
-  other_job_status_ = job.job_status_;
-  other_job_alternative_service_ = job.alternative_service_;
-  other_job_alternative_proxy_server_ = job.alternative_proxy_server_;
-
-  // At most one job can have a valid |alternative_proxy_server_|.
-  DCHECK(!alternative_proxy_server_.is_valid() ||
-         !other_job_alternative_proxy_server_.is_valid());
-
-  MaybeMarkAlternativeServiceBroken();
-}
-
-void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
-  // At least one job should not be an alternative job.
-  DCHECK(alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL ||
-         other_job_alternative_service_.protocol ==
-             UNINITIALIZED_ALTERNATE_PROTOCOL);
-
-  if (job_status_ == STATUS_RUNNING || other_job_status_ == STATUS_RUNNING)
-    return;
-
-  MaybeNotifyAlternativeProxyServerBroken();
-
-  if (IsSpdyAlternative() || IsQuicAlternative()) {
-    if (job_status_ == STATUS_BROKEN && other_job_status_ == STATUS_SUCCEEDED) {
-      HistogramBrokenAlternateProtocolLocation(
-          BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_IMPL_JOB_ALT);
-      session_->http_server_properties()->MarkAlternativeServiceBroken(
-          alternative_service_);
-    }
-    return;
-  }
-
-  session_->quic_stream_factory()->OnTcpJobCompleted(job_status_ ==
-                                                     STATUS_SUCCEEDED);
-  if (job_status_ == STATUS_SUCCEEDED && other_job_status_ == STATUS_BROKEN) {
-    HistogramBrokenAlternateProtocolLocation(
-        BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_IMPL_JOB_MAIN);
-    session_->http_server_properties()->MarkAlternativeServiceBroken(
-        other_job_alternative_service_);
-  }
-}
-
-void HttpStreamFactoryImpl::Job::MaybeNotifyAlternativeProxyServerBroken()
-    const {
-  if (!alternative_proxy_server_.is_valid() &&
-      !other_job_alternative_proxy_server_.is_valid()) {
-    // Neither of the two jobs used an alternative proxy server.
-    return;
-  }
-
-  // Neither this job, nor the other job should have used the alternative
-  // service.
-  DCHECK_EQ(UNINITIALIZED_ALTERNATE_PROTOCOL, alternative_service_.protocol);
-  DCHECK_EQ(UNINITIALIZED_ALTERNATE_PROTOCOL,
-            other_job_alternative_service_.protocol);
-
-  ProxyDelegate* proxy_delegate = session_->params().proxy_delegate;
-  if (!proxy_delegate)
-    return;
-
-  if (alternative_proxy_server_.is_valid()) {
-    // |this| connected to the alternative proxy server.
-    if ((job_status_ == STATUS_BROKEN || job_status_ == STATUS_FAILED) &&
-        other_job_status_ == STATUS_SUCCEEDED) {
-      // Notify ProxyDelegate.
-      proxy_delegate->OnAlternativeProxyBroken(alternative_proxy_server_);
-    }
-    return;
-  }
-
-  if (other_job_alternative_proxy_server_.is_valid()) {
-    // Other job connected to the alternative proxy server.
-    if (job_status_ == STATUS_SUCCEEDED &&
-        (other_job_status_ == STATUS_BROKEN ||
-         other_job_status_ == STATUS_FAILED)) {
-      // Notify ProxyDelegate.
-      proxy_delegate->OnAlternativeProxyBroken(
-          other_job_alternative_proxy_server_);
-    }
-    return;
-  }
-}
-
 ClientSocketPoolManager::SocketGroupType
 HttpStreamFactoryImpl::Job::GetSocketGroup() const {
   std::string scheme = origin_url_.scheme();
   if (scheme == "https" || scheme == "wss" || IsSpdyAlternative())
     return ClientSocketPoolManager::SSL_GROUP;
 
   if (scheme == "ftp")
     return ClientSocketPoolManager::FTP_GROUP;
 
   return ClientSocketPoolManager::NORMAL_GROUP;
@@ skipping 11 matching lines @@
 
   ConnectionAttempts socket_attempts = connection_->connection_attempts();
   if (connection_->socket()) {
     connection_->socket()->GetConnectionAttempts(&socket_attempts);
   }
 
   delegate_->AddConnectionAttemptsToRequest(this, socket_attempts);
 }
 
 }  // namespace net