Remove HttpStreamFactoryImpl::Job::MarkAsAlternate.

* Add AlternativeService argument to Job::Job() to signal whether Job is
  alternative;
* remove HttpStreamFactoryImpl::Job::MarkAsAlternate() method;
* add const bool is_spdy_alternative_ and is_quic_alternative members;
* remove Job::IsAlternate() and IsSpdyAlternate() methods;
* s/alternate/alternative/ in comments.

BUG=489280

Review URL: https://codereview.chromium.org/1162893003

Cr-Commit-Position: refs/heads/master@{#332820}

net/http/http_server_properties.h

@@ -111,7 +111,7 @@ struct NET_EXPORT AlternativeService {
   AlternativeService& operator=(const AlternativeService& alternative_service) =
       default;
 
-  HostPortPair host_port_pair() { return HostPortPair(host, port); }
+  HostPortPair host_port_pair() const { return HostPortPair(host, port); }
 
   bool operator==(const AlternativeService& other) const {
     return protocol == other.protocol && host == other.host &&

net/http/http_stream_factory_impl.cc

@@ -99,22 +99,21 @@ HttpStreamRequest* HttpStreamFactoryImpl::RequestStreamInternal(
     // Never share connection with other jobs for FTP requests.
     DCHECK(!request_info.url.SchemeIs("ftp"));
 
-    Job* alternate_job =
+    Job* alternative_job =
         new Job(this, session_, request_info, priority, server_ssl_config,
-                proxy_ssl_config, net_log.net_log());
-    request->AttachJob(alternate_job);
-    alternate_job->MarkAsAlternate(alternative_service);
+                proxy_ssl_config, alternative_service, net_log.net_log());
+    request->AttachJob(alternative_job);
 
-    job->WaitFor(alternate_job);
+    job->WaitFor(alternative_job);
     // Make sure to wait until we call WaitFor(), before starting
-    // |alternate_job|, otherwise |alternate_job| will not notify |job|
+    // |alternative_job|, otherwise |alternative_job| will not notify |job|
     // appropriately.
-    alternate_job->Start(request);
+    alternative_job->Start(request);
   }
 
-  // Even if |alternate_job| has already finished, it won't have notified the
-  // request yet, since we defer that to the next iteration of the MessageLoop,
-  // so starting |job| is always safe.
+  // Even if |alternative_job| has already finished, it will not have notified
+  // the request yet, since we defer that to the next iteration of the
+  // MessageLoop, so starting |job| is always safe.
   job->Start(request);
   return request;
 }
@@ -128,11 +127,9 @@ void HttpStreamFactoryImpl::PreconnectStreams(
   DCHECK(!for_websockets_);
   AlternativeService alternative_service =
       GetAlternativeServiceFor(request_info.url);
-  Job* job = new Job(this, session_, request_info, priority, server_ssl_config,
-                     proxy_ssl_config, session_->net_log());
-  if (alternative_service.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL) {
-    job->MarkAsAlternate(alternative_service);
-  }
+  Job* job =
+      new Job(this, session_, request_info, priority, server_ssl_config,
+              proxy_ssl_config, alternative_service, session_->net_log());
   preconnect_job_set_.insert(job);
   job->Preconnect(num_streams);
 }

net/http/http_stream_factory_impl_job.cc

@@ -80,6 +80,24 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
                                 const SSLConfig& server_ssl_config,
                                 const SSLConfig& proxy_ssl_config,
                                 NetLog* net_log)
+    : Job(stream_factory,
+          session,
+          request_info,
+          priority,
+          server_ssl_config,
+          proxy_ssl_config,
+          AlternativeService(),
+          net_log) {
+}
+
+HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
+                                HttpNetworkSession* session,
+                                const HttpRequestInfo& request_info,
+                                RequestPriority priority,
+                                const SSLConfig& server_ssl_config,
+                                const SSLConfig& proxy_ssl_config,
+                                AlternativeService alternative_service,
+                                NetLog* net_log)
     : request_(NULL),
       request_info_(request_info),
       priority_(priority),
@@ -92,6 +110,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       stream_factory_(stream_factory),
       next_state_(STATE_NONE),
       pac_request_(NULL),
+      alternative_service_(alternative_service),
       blocking_job_(NULL),
       waiting_job_(NULL),
       using_ssl_(false),
@@ -110,6 +129,10 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       ptr_factory_(this) {
   DCHECK(stream_factory);
   DCHECK(session);
+  if (IsQuicAlternative()) {
+    DCHECK(session_->params().enable_quic);
+    using_quic_ = true;
+  }
 }
 
 HttpStreamFactoryImpl::Job::~Job() {
@@ -171,16 +194,6 @@ LoadState HttpStreamFactoryImpl::Job::GetLoadState() const {
   }
 }
 
-void HttpStreamFactoryImpl::Job::MarkAsAlternate(
-    AlternativeService alternative_service) {
-  DCHECK(!IsAlternate());
-  alternative_service_ = alternative_service;
-  if (alternative_service.protocol == QUIC) {
-    DCHECK(session_->params().enable_quic);
-    using_quic_ = true;
-  }
-}
-
 void HttpStreamFactoryImpl::Job::WaitFor(Job* job) {
   DCHECK_EQ(STATE_NONE, next_state_);
   DCHECK_EQ(STATE_NONE, job->next_state_);
@@ -289,7 +302,7 @@ bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
   // TODO(ricea): Add "wss" back to this list when SPDY WebSocket support is
   // working.
   return origin_url_.SchemeIs("https") ||
-         proxy_info_.proxy_server().is_https() || IsSpdyAlternate();
+         proxy_info_.proxy_server().is_https() || IsSpdyAlternative();
 }
 
 void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
@@ -555,7 +568,7 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
 
     default:
       DCHECK(result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN ||
-             IsAlternate());
+             IsSpdyAlternative() || IsQuicAlternative());
       if (job_status_ != STATUS_BROKEN) {
         DCHECK_EQ(STATUS_RUNNING, job_status_);
         job_status_ = STATUS_FAILED;
@@ -639,7 +652,7 @@ int HttpStreamFactoryImpl::Job::StartInternal() {
 }
 
 int HttpStreamFactoryImpl::Job::DoStart() {
-  if (IsAlternate()) {
+  if (IsSpdyAlternative() || IsQuicAlternative()) {
     server_ = alternative_service_.host_port_pair();
   } else {
     server_ = HostPortPair::FromURL(request_info_.url);
@@ -647,7 +660,7 @@ int HttpStreamFactoryImpl::Job::DoStart() {
   origin_url_ =
       stream_factory_->ApplyHostMappingRules(request_info_.url, &server_);
   valid_spdy_session_pool_.reset(new ValidSpdySessionPool(
-      session_->spdy_session_pool(), origin_url_, IsSpdyAlternate()));
+      session_->spdy_session_pool(), origin_url_, IsSpdyAlternative()));
 
   net_log_.BeginEvent(
       NetLog::TYPE_HTTP_STREAM_JOB,
@@ -691,7 +704,7 @@ int HttpStreamFactoryImpl::Job::DoResolveProxy() {
   // https://<alternative host>:<alternative port>/...
   // so the proxy resolution works with the actual destination, and so
   // that the correct socket pool is used.
-  if (IsSpdyAlternate()) {
+  if (IsSpdyAlternative()) {
     // TODO(rch):  Figure out how to make QUIC iteract with PAC
     // scripts.  By not re-writing the URL, we will query the PAC script
     // for the proxy to use to reach the original URL via TCP.  But
@@ -732,7 +745,7 @@ int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
     } else if (using_quic_ &&
                (!proxy_info_.is_quic() && !proxy_info_.is_direct())) {
       // QUIC can not be spoken to non-QUIC proxies.  This error should not be
-      // user visible, because the non-alternate job should be resumed.
+      // user visible, because the non-alternative Job should be resumed.
       result = ERR_NO_SUPPORTED_PROXIES;
     }
   }
@@ -782,7 +795,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
   using_ssl_ = origin_url_.SchemeIs("https") || origin_url_.SchemeIs("wss") ||
-               IsSpdyAlternate();
+               IsSpdyAlternative();
   using_spdy_ = false;
 
   if (ShouldForceQuic())
@@ -858,7 +871,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     establishing_tunnel_ = using_ssl_;
 
   // TODO(bnc): s/want_spdy_over_npn/expect_spdy_over_npn/
-  bool want_spdy_over_npn = IsAlternate();
+  bool want_spdy_over_npn = IsSpdyAlternative();
 
   if (proxy_info_.is_https()) {
     InitSSLConfig(proxy_info_.proxy_server().host_port_pair(),
@@ -1014,13 +1027,14 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
     return result;
   }
 
-  if (IsSpdyAlternate() && !using_spdy_) {
+  if (IsSpdyAlternative() && !using_spdy_) {
     job_status_ = STATUS_BROKEN;
     MaybeMarkAlternativeServiceBroken();
     return ERR_NPN_NEGOTIATION_FAILED;
   }
 
-  if (!ssl_started && result < 0 && IsAlternate()) {
+  if (!ssl_started && result < 0 &&
+      (IsSpdyAlternative() || IsQuicAlternative())) {
     job_status_ = STATUS_BROKEN;
     // TODO(bnc): if (result == ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN), then
     // instead of marking alternative service broken, mark (origin, alternative
@@ -1056,7 +1070,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
   if (using_ssl_) {
     DCHECK(ssl_started);
     if (IsCertificateError(result)) {
-      if (using_spdy_ && IsAlternate() && origin_url_.SchemeIs("http")) {
+      if (IsSpdyAlternative() && origin_url_.SchemeIs("http")) {
         // We ignore certificate errors for http over spdy.
         spdy_certificate_error_ = result;
         result = OK;
@@ -1107,8 +1121,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462811 HttpStreamFactoryImpl::Job::DoCreateStream"));
   DCHECK(connection_->socket() || existing_spdy_session_.get() || using_quic_);
-  if (IsAlternate())
-    DCHECK(IsSpdyAlternate());
+  DCHECK(!IsQuicAlternative());
 
   next_state_ = STATE_CREATE_STREAM_COMPLETE;
 
@@ -1119,7 +1132,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     SetSocketMotivation();
 
   if (!using_spdy_) {
-    DCHECK(!IsSpdyAlternate());
+    DCHECK(!IsSpdyAlternative());
     // We may get ftp scheme when fetching ftp resources through proxy.
     bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
                        (request_info_.url.SchemeIs("http") ||
@@ -1249,7 +1262,7 @@ void HttpStreamFactoryImpl::Job::SetSocketMotivation() {
 bool HttpStreamFactoryImpl::Job::IsHttpsProxyAndHttpUrl() const {
   if (!proxy_info_.is_https())
     return false;
-  if (IsAlternate()) {
+  if (IsSpdyAlternative() || IsQuicAlternative()) {
     // We currently only support Alternate-Protocol where the original scheme
     // is http.
     DCHECK(origin_url_.SchemeIs("http"));
@@ -1258,15 +1271,15 @@ bool HttpStreamFactoryImpl::Job::IsHttpsProxyAndHttpUrl() const {
   return request_info_.url.SchemeIs("http");
 }
 
-bool HttpStreamFactoryImpl::Job::IsAlternate() const {
-  return alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL;
-}
-
-bool HttpStreamFactoryImpl::Job::IsSpdyAlternate() const {
+bool HttpStreamFactoryImpl::Job::IsSpdyAlternative() const {
   return alternative_service_.protocol >= NPN_SPDY_MINIMUM_VERSION &&
          alternative_service_.protocol <= NPN_SPDY_MAXIMUM_VERSION;
 }
 
+bool HttpStreamFactoryImpl::Job::IsQuicAlternative() const {
+  return alternative_service_.protocol == QUIC;
+}
+
 void HttpStreamFactoryImpl::Job::InitSSLConfig(const HostPortPair& server,
                                                SSLConfig* ssl_config,
                                                bool is_proxy) const {
@@ -1437,12 +1450,11 @@ void HttpStreamFactoryImpl::Job::ReportJobSucceededForRequest() {
     // If an existing session was used, then no TCP connection was
     // started.
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE);
-  } else if (IsAlternate()) {
-    // This job was the alternate protocol job, and hence won the 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 alternate protocol job lost
-    // the race.
+    // This Job was the normal Job, and hence the alternative Job lost the race.
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE);
   }
 }
@@ -1458,7 +1470,7 @@ void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
   if (job_status_ == STATUS_RUNNING || other_job_status_ == STATUS_RUNNING)
     return;
 
-  if (IsAlternate()) {
+  if (IsSpdyAlternative() || IsQuicAlternative()) {
     if (job_status_ == STATUS_BROKEN && other_job_status_ == STATUS_SUCCEEDED) {
       HistogramBrokenAlternateProtocolLocation(
           BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_IMPL_JOB_ALT);
@@ -1479,10 +1491,10 @@ void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
 HttpStreamFactoryImpl::Job::ValidSpdySessionPool::ValidSpdySessionPool(
     SpdySessionPool* spdy_session_pool,
     GURL& origin_url,
-    bool is_spdy_alternate)
+    bool is_spdy_alternative)
     : spdy_session_pool_(spdy_session_pool),
       origin_url_(origin_url),
-      is_spdy_alternate_(is_spdy_alternate) {
+      is_spdy_alternative_(is_spdy_alternative) {
 }
 
 int HttpStreamFactoryImpl::Job::ValidSpdySessionPool::FindAvailableSession(
@@ -1508,10 +1520,10 @@ int HttpStreamFactoryImpl::Job::ValidSpdySessionPool::
 int HttpStreamFactoryImpl::Job::ValidSpdySessionPool::
     CheckAlternativeServiceValidityForOrigin(
         base::WeakPtr<SpdySession> spdy_session) {
-  // For a SPDY alternate Job, server_.host() might be different than
+  // For an alternative Job, server_.host() might be different than
   // origin_url_.host(), therefore it needs to be verified that the former
   // provides a certificate that is valid for the latter.
-  if (!is_spdy_alternate_ || !spdy_session ||
+  if (!is_spdy_alternative_ || !spdy_session ||
       spdy_session->VerifyDomainAuthentication(origin_url_.host())) {
     return OK;
   }
@@ -1521,7 +1533,7 @@ int HttpStreamFactoryImpl::Job::ValidSpdySessionPool::
 ClientSocketPoolManager::SocketGroupType
 HttpStreamFactoryImpl::Job::GetSocketGroup() const {
   std::string scheme = origin_url_.scheme();
-  if (scheme == "https" || scheme == "wss" || IsSpdyAlternate())
+  if (scheme == "https" || scheme == "wss" || IsSpdyAlternative())
     return ClientSocketPoolManager::SSL_GROUP;
 
   if (scheme == "ftp")

net/http/http_stream_factory_impl_job.h

@@ -36,6 +36,7 @@ class QuicHttpStream;
 // created for the StreamFactory.
 class HttpStreamFactoryImpl::Job {
  public:
+  // Constructor for non-alternative Job.
   Job(HttpStreamFactoryImpl* stream_factory,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
@@ -43,6 +44,15 @@ class HttpStreamFactoryImpl::Job {
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       NetLog* net_log);
+  // Constructor for alternative Job.
+  Job(HttpStreamFactoryImpl* stream_factory,
+      HttpNetworkSession* session,
+      const HttpRequestInfo& request_info,
+      RequestPriority priority,
+      const SSLConfig& server_ssl_config,
+      const SSLConfig& proxy_ssl_config,
+      AlternativeService alternative_service,
+      NetLog* net_log);
   ~Job();
 
   // Start initiates the process of creating a new HttpStream. |request| will be
@@ -56,10 +66,6 @@ class HttpStreamFactoryImpl::Job {
   int RestartTunnelWithProxyAuth(const AuthCredentials& credentials);
   LoadState GetLoadState() const;
 
-  // Marks this Job as the "alternate" job, from Alternate-Protocol or Alt-Svc
-  // using the specified alternate service.
-  void MarkAsAlternate(AlternativeService alternative_service);
-
   // Tells |this| to wait for |job| to resume it.
   void WaitFor(Job* job);
 
@@ -146,13 +152,14 @@ class HttpStreamFactoryImpl::Job {
    public:
     ValidSpdySessionPool(SpdySessionPool* spdy_session_pool,
                          GURL& origin_url,
-                         bool is_spdy_alternate);
+                         bool is_spdy_alternative);
 
     // Returns OK if a SpdySession was not found (in which case |spdy_session|
     // is set to nullptr), or if one was found (in which case |spdy_session| is
     // set to it) and it has an associated SSL certificate with is valid for
     // |origin_url_|, or if this requirement does not apply because the Job is
-    // not a SPDY alternate job.  Returns the appropriate error code otherwise,
+    // not a SPDY alternative job.  Returns the appropriate error code
+    // otherwise,
     // in which case |spdy_session| should not be used.
     int FindAvailableSession(const SpdySessionKey& key,
                              const BoundNetLog& net_log,
@@ -160,7 +167,7 @@ class HttpStreamFactoryImpl::Job {
 
     // Creates a SpdySession and sets |spdy_session| to point to it.  Returns OK
     // if the associated SSL certificate is valid for |origin_url_|, or if this
-    // requirement does not apply because the Job is not a SPDY alternate job.
+    // requirement does not apply because the Job is not a SPDY alternative job.
     // Returns the appropriate error code otherwise, in which case
     // |spdy_session| should not be used.
     int CreateAvailableSessionFromSocket(
@@ -174,14 +181,14 @@ class HttpStreamFactoryImpl::Job {
    private:
     // Returns OK if |spdy_session| has an associated SSL certificate with is
     // valid for |origin_url_|, or if this requirement does not apply because
-    // the Job is not a SPDY alternate job, or if |spdy_session| is null.
+    // the Job is not a SPDY alternative job, or if |spdy_session| is null.
     // Returns appropriate error code otherwise.
     int CheckAlternativeServiceValidityForOrigin(
         base::WeakPtr<SpdySession> spdy_session);
 
     SpdySessionPool* const spdy_session_pool_;
     const GURL origin_url_;
-    const bool is_spdy_alternate_;
+    const bool is_spdy_alternative_;
   };
 
   void OnStreamReadyCallback();
@@ -231,12 +238,9 @@ class HttpStreamFactoryImpl::Job {
 
   bool IsHttpsProxyAndHttpUrl() const;
 
-  // Returns true iff this Job is an alternate, that is, iff MarkAsAlternate has
-  // been called.
-  bool IsAlternate() const;
-
-  // Returns true if this Job is a SPDY alternate job.
-  bool IsSpdyAlternate() const;
+  // Is this a SPDY or QUIC alternative Job?
+  bool IsSpdyAlternative() const;
+  bool IsQuicAlternative() const;
 
   // Sets several fields of |ssl_config| for |server| based on the proxy info
   // and other factors.
@@ -319,10 +323,10 @@ class HttpStreamFactoryImpl::Job {
   // original request when host mapping rules are set-up.
   GURL origin_url_;
 
-  // AlternateProtocol for this job if this is an alternate job.
-  AlternativeService alternative_service_;
+  // AlternativeService for this Job if this is an alternative Job.
+  const AlternativeService alternative_service_;
 
-  // AlternateProtocol for the other job if this is not an alternate job.
+  // AlternativeService for the other Job if this is not an alternative Job.
   AlternativeService other_job_alternative_service_;
 
   // This is the Job we're dependent on. It will notify us if/when it's OK to