Minor cleanups in HttpStreamFactoryImpl and member classes.

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>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
@@ skipping 27 matching lines @@
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/ssl_client_socket_pool.h"
 #include "net/spdy/bidirectional_stream_spdy_impl.h"
 #include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_protocol.h"
 #include "net/spdy/spdy_session.h"
 #include "net/spdy/spdy_session_pool.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_connection_status_flags.h"
+#include "url/url_constants.h"
 
 namespace net {
 
 namespace {
 
-void DoNothingAsyncCallback(int result){};
+void DoNothingAsyncCallback(int result) {}
 void RecordChannelIDKeyMatch(SSLClientSocket* ssl_socket,
                              ChannelIDService* channel_id_service,
                              std::string host) {
   SSLInfo ssl_info;
   ssl_socket->GetSSLInfo(&ssl_info);
   if (!ssl_info.channel_id_sent)
     return;
   std::unique_ptr<crypto::ECPrivateKey> request_key;
   ChannelIDService::Request request;
   int result = channel_id_service->GetOrCreateChannelID(
@@ skipping 149 matching lines @@
   // alternative proxy server is valid, then the job type must be set to
   // either ALTERNATIVE or PRECONNECT.
   DCHECK((alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL &&
           !alternative_proxy_server_.is_valid()) ||
          (job_type_ == ALTERNATIVE || job_type_ == PRECONNECT));
   // If the alternative proxy server is valid, then the job type must be
   // set to ALTERNATIVE.
   DCHECK(!alternative_proxy_server_.is_valid() || job_type_ == ALTERNATIVE);
 
   if (IsSpdyAlternative()) {
-    DCHECK(origin_url_.SchemeIs("https"));
+    DCHECK(origin_url_.SchemeIs(url::kHttpsScheme));
   }
   if (IsQuicAlternative()) {
     DCHECK(session_->params().enable_quic);
     using_quic_ = true;
   }
 }
 
 HttpStreamFactoryImpl::Job::~Job() {
   net_log_.EndEvent(NetLogEventType::HTTP_STREAM_JOB);
 
@@ skipping 126 matching lines @@
 }
 
 bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
   // We need to make sure that if a spdy session was created for
   // https://somehost/ that we don't use that session for http://somehost:443/.
   // The only time we can use an existing session is if the request URL is
   // https (the normal case) or if we're connection to a SPDY proxy.
   // https://crbug.com/133176
   // 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() || IsSpdyAlternative();

[Bence, 2016/09/22 19:30:32]

See DCHECK in line 232.

+  return origin_url_.SchemeIs(url::kHttpsScheme) ||
+         proxy_info_.proxy_server().is_https();
 }
 
 void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
   DCHECK(stream_.get());
   DCHECK_NE(job_type_, PRECONNECT);
   DCHECK(!delegate_->for_websockets());
 
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
   delegate_->OnStreamReady(this, server_ssl_config_, proxy_info_);
@@ skipping 376 matching lines @@
 
   return OK;
 }
 
 bool HttpStreamFactoryImpl::Job::ShouldForceQuic() const {
   return session_->params().enable_quic &&
          (base::ContainsKey(session_->params().origins_to_force_quic_on,
                             HostPortPair()) ||
           base::ContainsKey(session_->params().origins_to_force_quic_on,
                             destination_)) &&
-         proxy_info_.is_direct() && origin_url_.SchemeIs("https");
+         proxy_info_.is_direct() && origin_url_.SchemeIs(url::kHttpsScheme);
 }
 
 int HttpStreamFactoryImpl::Job::DoWait() {
   next_state_ = STATE_WAIT_COMPLETE;
   if (delegate_->ShouldWait(this))
     return ERR_IO_PENDING;
 
   return OK;
 }
 
@@ skipping 13 matching lines @@
 
 int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/462812 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462812 HttpStreamFactoryImpl::Job::DoInitConnection"));
   DCHECK(!connection_->is_initialized());
   DCHECK(proxy_info_.proxy_server().is_valid());
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
-  using_ssl_ = origin_url_.SchemeIs("https") || origin_url_.SchemeIs("wss") ||
-               IsSpdyAlternative();
+  using_ssl_ = origin_url_.SchemeIs(url::kHttpsScheme) ||
+               origin_url_.SchemeIs(url::kWssScheme);
   using_spdy_ = false;
 
   if (ShouldForceQuic())
     using_quic_ = true;
 
   DCHECK(!using_quic_ || session_->params().enable_quic);
 
   if (proxy_info_.is_quic()) {
     using_quic_ = true;
     DCHECK(session_->params().enable_quic);
   }
 
   if (proxy_info_.is_https() || proxy_info_.is_quic()) {
     InitSSLConfig(&proxy_ssl_config_, /*is_proxy=*/true);
     // Disable revocation checking for HTTPS proxies since the revocation
     // requests are probably going to need to go through the proxy too.
     proxy_ssl_config_.rev_checking_enabled = false;
   }
   if (using_ssl_) {
     InitSSLConfig(&server_ssl_config_, /*is_proxy=*/false);
   }
 
   if (using_quic_) {
-    if (proxy_info_.is_quic() && !request_info_.url.SchemeIs("http")) {
+    if (proxy_info_.is_quic() &&
+        !request_info_.url.SchemeIs(url::kHttpScheme)) {
       NOTREACHED();
       // TODO(rch): support QUIC proxies for HTTPS urls.
       return ERR_NOT_IMPLEMENTED;
     }
     HostPortPair destination;
     SSLConfig* ssl_config;
     GURL url(request_info_.url);
     if (proxy_info_.is_quic()) {
       // A proxy's certificate is expected to be valid for the proxy hostname.
       destination = proxy_info_.proxy_server().host_port_pair();
       ssl_config = &proxy_ssl_config_;
       GURL::Replacements replacements;
-      replacements.SetSchemeStr("https");
+      replacements.SetSchemeStr(url::kHttpsScheme);
       replacements.SetHostStr(destination.host());
       const std::string new_port = base::UintToString(destination.port());
       replacements.SetPortStr(new_port);
       replacements.ClearUsername();
       replacements.ClearPassword();
       replacements.ClearPath();
       replacements.ClearQuery();
       replacements.ClearRef();
       url = url.ReplaceComponents(replacements);
 
@@ skipping 117 matching lines @@
     if (existing_spdy_session_) {
       using_spdy_ = true;
       next_state_ = STATE_CREATE_STREAM;
     } else {
       // It is possible that the spdy session no longer exists.
       ReturnToStateInitConnection(true /* close connection */);
     }
     return OK;
   }
 
-  if (proxy_info_.is_quic() && using_quic_) {
+  if (proxy_info_.is_quic()) {
+    DCHECK(using_quic_);

[Bence, 2016/09/22 19:30:32]

See assignment in line 821.

     // Mark QUIC proxy as bad if QUIC got disabled.
     // Underlying QUIC layer would have closed the connection.
-    HostPortPair destination = proxy_info_.proxy_server().host_port_pair();
     if (session_->quic_stream_factory()->IsQuicDisabled()) {
       using_quic_ = false;
       return ReconsiderProxyAfterError(ERR_QUIC_PROTOCOL_ERROR);
     }
   }
 
   // |result| may be the result of any of the stacked pools. The following
   // logic is used when determining how to interpret an error.
   // If |result| < 0:
   //   and connection_->socket() != NULL, then the SSL handshake ran and it
@@ skipping 11 matching lines @@
     } else {
       SSLClientSocket* ssl_socket =
           static_cast<SSLClientSocket*>(connection_->socket());
       if (ssl_socket->WasNpnNegotiated()) {
         was_alpn_negotiated_ = true;
         negotiated_protocol_ = ssl_socket->GetNegotiatedProtocol();
         net_log_.AddEvent(
             NetLogEventType::HTTP_STREAM_REQUEST_PROTO,
             base::Bind(&NetLogHttpStreamProtoCallback, negotiated_protocol_));
         if (negotiated_protocol_ == kProtoHTTP2)
-          SwitchToSpdyMode();
+          using_spdy_ = true;
       }
     }
   } else if (proxy_info_.is_https() && connection_->socket() &&
         result == OK) {
     ProxyClientSocket* proxy_socket =
       static_cast<ProxyClientSocket*>(connection_->socket());
     // http://crbug.com/642354
     if (!proxy_socket->IsConnected())
       return ERR_CONNECTION_CLOSED;
     if (proxy_socket->IsUsingSpdy()) {
       was_alpn_negotiated_ = true;
       negotiated_protocol_ = proxy_socket->GetProxyNegotiatedProtocol();
-      SwitchToSpdyMode();
+      using_spdy_ = true;
     }
   }
 
   if (result == ERR_PROXY_AUTH_REQUESTED ||
       result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
     DCHECK(!ssl_started);
     // Other state (i.e. |using_ssl_|) suggests that |connection_| will have an
     // SSL socket, but there was an error before that could happen.  This
     // puts the in progress HttpProxy socket into |connection_| in order to
     // complete the auth (or read the response body).  The tunnel restart code
@@ skipping 38 matching lines @@
   }
 
   if (result < 0 && !ssl_started)
     return ReconsiderProxyAfterError(result);
   establishing_tunnel_ = false;
 
   // Handle SSL errors below.
   if (using_ssl_) {
     DCHECK(ssl_started);
     if (IsCertificateError(result)) {
-      if (IsSpdyAlternative() && origin_url_.SchemeIs("http")) {

[Bence, 2016/09/22 19:30:32]

See DCHECK in line 232.

-        // We ignore certificate errors for http over spdy.
-        spdy_certificate_error_ = result;
-        result = OK;
-      } else {
-        result = HandleCertificateError(result);
-        if (result == OK && !connection_->socket()->IsConnectedAndIdle()) {
-          ReturnToStateInitConnection(true /* close connection */);
-          return result;
-        }
+      result = HandleCertificateError(result);
+      if (result == OK && !connection_->socket()->IsConnectedAndIdle()) {
+        ReturnToStateInitConnection(true /* close connection */);
+        return result;
       }
     }
     if (result < 0)
       return result;
   }
 
   next_state_ = STATE_CREATE_STREAM;
   return OK;
 }
 
@@ skipping 15 matching lines @@
     return ERR_NOT_IMPLEMENTED;
   if (stream_type_ == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
     bidirectional_stream_impl_.reset(new BidirectionalStreamSpdyImpl(session));
     return OK;
   }
 
   // TODO(willchan): Delete this code, because eventually, the
   // HttpStreamFactoryImpl will be creating all the SpdyHttpStreams, since it
   // will know when SpdySessions become available.
 
-  bool use_relative_url = direct || request_info_.url.SchemeIs("https");
+  bool use_relative_url =
+      direct || request_info_.url.SchemeIs(url::kHttpsScheme);
   stream_.reset(new SpdyHttpStream(session, use_relative_url));
   return OK;
 }
 
 int HttpStreamFactoryImpl::Job::DoCreateStream() {
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/462811 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462811 HttpStreamFactoryImpl::Job::DoCreateStream"));
   DCHECK(connection_->socket() || existing_spdy_session_.get() || using_quic_);
@@ skipping 11 matching lines @@
   // We only set the socket motivation if we're the first to use
   // this socket.  Is there a race for two SPDY requests?  We really
   // need to plumb this through to the connect level.
   if (connection_->socket() && !connection_->is_reused())
     SetSocketMotivation();
 
   if (!using_spdy_) {
     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") ||
-                        request_info_.url.SchemeIs("ftp"));
+                       (request_info_.url.SchemeIs(url::kHttpScheme) ||
+                        request_info_.url.SchemeIs(url::kFtpScheme));
     if (delegate_->for_websockets()) {
       DCHECK_NE(job_type_, PRECONNECT);
       DCHECK(delegate_->websocket_handshake_stream_create_helper());
       websocket_stream_.reset(
           delegate_->websocket_handshake_stream_create_helper()
               ->CreateBasicStream(std::move(connection_), using_proxy));
     } else {
       stream_.reset(new HttpBasicStream(
           std::move(connection_), using_proxy,
           session_->params().http_09_on_non_default_ports_enabled));
@@ skipping 39 matching lines @@
   if (spdy_session->GetSSLInfo(&ssl_info)) {
     UMA_HISTOGRAM_SPARSE_SLOWLY(
         "Net.Http2SSLCipherSuite",
         SSLConnectionStatusToCipherSuite(ssl_info.connection_status));
   }
 
   new_spdy_session_ = spdy_session;
   spdy_session_direct_ = direct;
   const HostPortPair host_port_pair = spdy_session_key.host_port_pair();
   bool is_https = ssl_info.is_valid();
-  url::SchemeHostPort scheme_host_port(is_https ? "https" : "http",
-                                       host_port_pair.host(),
-                                       host_port_pair.port());
+  url::SchemeHostPort scheme_host_port(
+      is_https ? url::kHttpsScheme : url::kHttpScheme, host_port_pair.host(),
+      host_port_pair.port());
 
   HttpServerProperties* http_server_properties =
       session_->http_server_properties();
   if (http_server_properties)
     http_server_properties->SetSupportsSpdy(scheme_host_port, true);
 
   // Create a SpdyHttpStream or a BidirectionalStreamImpl attached to the
   // session; OnNewSpdySessionReadyCallback is not called until an event loop
   // iteration later, so if the SpdySession is closed between then, allow
   // reuse state from the underlying socket, sampled by SpdyHttpStream,
@@ skipping 53 matching lines @@
   if (request_info_.motivation == HttpRequestInfo::PRECONNECT_MOTIVATED)
     connection_->socket()->SetSubresourceSpeculation();
   else if (request_info_.motivation == HttpRequestInfo::OMNIBOX_MOTIVATED)
     connection_->socket()->SetOmniboxSpeculation();
   // TODO(mbelshe): Add other motivations (like EARLY_LOAD_MOTIVATED).
 }
 
 bool HttpStreamFactoryImpl::Job::IsHttpsProxyAndHttpUrl() const {
   if (!proxy_info_.is_https())
     return false;
-  if (IsSpdyAlternative() || IsQuicAlternative()) {

[Bence, 2016/09/22 19:30:32]

See DCHECK in line 232 and the one below.

+  DCHECK(!IsSpdyAlternative());
+  if (IsQuicAlternative()) {
     // We currently only support Alternate-Protocol where the original scheme

[Bence, 2016/09/22 19:30:32]

I think we do not support QUIC Alt-Svc either for "http" schemes.  Maybe remove
this if() here, and add DCHECK on scheme to line 235?

[Ryan Hamilton, 2016/09/22 21:38:05]

I confess, I don't understand this comment at all. I think it's just wrong.

[Bence, 2016/09/23 13:41:04]

I added a TODO here, I'd like to clean this up in a follow-up CL.

     // is http.
-    DCHECK(origin_url_.SchemeIs("http"));
-    return origin_url_.SchemeIs("http");
+    DCHECK(origin_url_.SchemeIs(url::kHttpScheme));
+    return origin_url_.SchemeIs(url::kHttpScheme);
   }
-  return request_info_.url.SchemeIs("http");
+  return request_info_.url.SchemeIs(url::kHttpScheme);
 }
 
 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;
 }
@@ skipping 53 matching lines @@
     case ERR_INTERNET_DISCONNECTED:
     case ERR_ADDRESS_UNREACHABLE:
     case ERR_CONNECTION_CLOSED:
     case ERR_CONNECTION_TIMED_OUT:
     case ERR_CONNECTION_RESET:
     case ERR_CONNECTION_REFUSED:
     case ERR_CONNECTION_ABORTED:
     case ERR_TIMED_OUT:
     case ERR_TUNNEL_CONNECTION_FAILED:
     case ERR_SOCKS_CONNECTION_FAILED:
     // This can happen in the case of trying to talk to a proxy using SSL, and

[Bence, 2016/09/22 19:30:32]

Does this comment and the next one each refer to the case right above or right
below?  I almost have the impression that they both refer to
|ERR_PROXY_CERTIFICATE_INVALID|.  I'm confused.

[Ryan Hamilton, 2016/09/22 21:38:05]

I think the second comment actually applies to:

 case ERR_SSL_PROTOCOL_ERROR:

[Bence, 2016/09/23 13:41:04]

Thanks.  I moved and refined the comments.

     // ending up talking to a captive portal that supports SSL instead.
     case ERR_PROXY_CERTIFICATE_INVALID:
-    // This can happen when trying to talk SSL to a non-SSL server (Like a
+    // This can happen when trying to talk SSL to a non-SSL server (like a
     // captive portal).
     case ERR_QUIC_PROTOCOL_ERROR:
     case ERR_QUIC_HANDSHAKE_FAILED:
     case ERR_MSG_TOO_BIG:
     case ERR_SSL_PROTOCOL_ERROR:
       break;
     case ERR_SOCKS_CONNECTION_HOST_UNREACHABLE:
       // Remap the SOCKS-specific "host unreachable" error to a more
       // generic error code (this way consumers like the link doctor
       // know to substitute their error page).
@@ skipping 71 matching lines @@
                                                     ssl_info_.cert_status);
 
   int load_flags = request_info_.load_flags;
   if (session_->params().ignore_certificate_errors)
     load_flags |= LOAD_IGNORE_ALL_CERT_ERRORS;
   if (ssl_socket->IgnoreCertError(error, load_flags))
     return OK;
   return error;
 }
 
-void HttpStreamFactoryImpl::Job::SwitchToSpdyMode() {
-  using_spdy_ = true;
-}
-
 void HttpStreamFactoryImpl::Job::ReportJobSucceededForRequest() {
   if (using_existing_quic_session_) {
     // If an existing session was used, then no TCP connection was
     // started.
     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);
   }
 }
 
 ClientSocketPoolManager::SocketGroupType
 HttpStreamFactoryImpl::Job::GetSocketGroup() const {
   std::string scheme = origin_url_.scheme();
-  if (scheme == "https" || scheme == "wss" || IsSpdyAlternative())
+  if (scheme == url::kHttpsScheme || scheme == url::kWssScheme)
     return ClientSocketPoolManager::SSL_GROUP;
 
-  if (scheme == "ftp")
+  if (scheme == url::kFtpScheme)
     return ClientSocketPoolManager::FTP_GROUP;
 
   return ClientSocketPoolManager::NORMAL_GROUP;
 }
 
 // If the connection succeeds, failed connection attempts leading up to the
 // success will be returned via the successfully connected socket. If the
 // connection fails, failed connection attempts will be returned via the
 // ClientSocketHandle. Check whether a socket was returned and copy the
 // connection attempts from the proper place.
 void HttpStreamFactoryImpl::Job::
     MaybeCopyConnectionAttemptsFromSocketOrHandle() {
   if (!connection_)
     return;
 
   ConnectionAttempts socket_attempts = connection_->connection_attempts();
   if (connection_->socket()) {
     connection_->socket()->GetConnectionAttempts(&socket_attempts);
   }
 
   delegate_->AddConnectionAttemptsToRequest(this, socket_attempts);
 }
 
 }  // namespace net