Minor cleanups in HttpStreamFactoryImpl and member classes.

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 bc4974565cac08be92bc61770b7ce70d0c8bd391..51f144ac20039f064e2b13e3d19636dd41c61820 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -6,7 +6,6 @@
 
 #include <algorithm>
 #include <string>
-#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -54,12 +53,13 @@
 #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) {
@@ -229,7 +229,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
   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);
@@ -376,8 +376,8 @@ bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
   // 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() {
@@ -774,7 +774,7 @@ bool HttpStreamFactoryImpl::Job::ShouldForceQuic() const {
                             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() {
@@ -808,8 +808,8 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
   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())
@@ -833,7 +833,8 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
   }
 
   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;
@@ -846,7 +847,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
       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);
@@ -984,10 +985,10 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
     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);
@@ -1019,7 +1020,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
             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() &&
@@ -1032,7 +1033,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
     if (proxy_socket->IsUsingSpdy()) {
       was_alpn_negotiated_ = true;
       negotiated_protocol_ = proxy_socket->GetProxyNegotiatedProtocol();
-      SwitchToSpdyMode();
+      using_spdy_ = true;
     }
   }
 
@@ -1091,16 +1092,10 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
   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)
@@ -1136,7 +1131,8 @@ int HttpStreamFactoryImpl::Job::SetSpdyHttpStreamOrBidirectionalStreamImpl(
   // 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;
 }
@@ -1168,8 +1164,8 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     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());
@@ -1229,9 +1225,9 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
   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();
@@ -1305,13 +1301,14 @@ void HttpStreamFactoryImpl::Job::SetSocketMotivation() {
 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 {
@@ -1388,7 +1385,7 @@ int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
     // 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:
@@ -1480,10 +1477,6 @@ int HttpStreamFactoryImpl::Job::HandleCertificateError(int error) {
   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
@@ -1501,10 +1494,10 @@ void HttpStreamFactoryImpl::Job::ReportJobSucceededForRequest() {
 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;