HttpStreamFactoryImpl::Job cleanup.

net/http/http_stream_factory_impl_job.h

Index: net/http/http_stream_factory_impl_job.h
diff --git a/net/http/http_stream_factory_impl_job.h b/net/http/http_stream_factory_impl_job.h
index 91c3def5cb8726f383f8909865f8b192d71b556f..cf20f917eea3073f71000c00f0839bb53d3ec6af 100644
--- a/net/http/http_stream_factory_impl_job.h
+++ b/net/http/http_stream_factory_impl_job.h
@@ -145,9 +145,28 @@ class HttpStreamFactoryImpl::Job {
     virtual bool for_websockets() = 0;
   };
 
-  // Constructor for non-alternative Job.
-  // Job is owned by |delegate|, hence |delegate| is valid for the
-  // lifetime of the Job.
+  // Job is owned by |delegate|, hence |delegate| is valid for the lifetime of
+  // the Job.
+  //
+  // |alternative_protocol| is the protocol required by Alternative Service, if
+  // any:
+  // * |alternative_protocol == kProtoUnknown| means that the Job can pool to an
+  //   existing SpdySession, or bind to a idle TCP socket that might use either
+  //   HTTP/1.1 or HTTP/2.
+  // * |alternative_protocol == kProtoHTTP2| means that the Job can pool to an
+  //   existing SpdySession, or bind to a idle TCP socket.  In the latter case,
+  //   if the socket does not use HTTP/2, then the Job fails.
+  // * |alternative_protocol == kProtoQUIC| means that the Job can pool to an
+  //   existing QUIC connection or open a new one.
+  // Note that this can be overwritten by using a QUIC proxy, or by setting

[xunjieli, 2017/06/14 18:28:01]

Just to be explicit. Could you specify that the QUIC proxy is done through
|alternative_proxy_server| ?

by using a QUIC proxy -> by using a QUIC proxy through
|alternative_proxy_server|.

[Bence, 2017/06/14 19:33:30]

It's good that you bring it up: in fact it is proxy_info_.is_quic() that forces
QUIC to be used.  In the old code that was in line 817, in the new code it's at
line 385, in the .cc file.

For an alternative proxy job, http_stream_factory_impl_job_controller.cc:831
fills |alternative_proxy_info| with |alternative_proxy_server|, and that
|alternative_proxy_info| gets passed to the factory as its |proxy_info|
argument.  So you are right that |alternative_proxy_server| can indirectly force
QUIC, though this seems to happen in JobController, not in Job.

+  // HttpNetworkSession::Params::origins_to_force_quic_on.
+  //
+  // If |alternative_proxy_server| is a valid proxy server, then the Job will
+  // use that instead of using ProxyService for proxy resolution.  Further, if
+  // |alternative_proxy_server| is a valid but bad proxy, then fallback proxies
+  // are not used. It is illegal to call this constructor with a valid
+  // |alternative_proxy_server| and an |alternate_protocol| different from
+  // kProtoUnknown.
   Job(Delegate* delegate,
       JobType job_type,
       HttpNetworkSession* session,
@@ -158,28 +177,7 @@ class HttpStreamFactoryImpl::Job {
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
-      bool enable_ip_based_pooling,
-      NetLog* net_log);
-
-  // Constructor for the alternative Job. The Job is owned by |delegate|, hence
-  // |delegate| is valid for the lifetime of the Job. If |alternative_service|
-  // is initialized, then the Job will use the alternative service. On the
-  // other hand, if |alternative_proxy_server| is a valid proxy server, then the
-  // job will use that instead of using ProxyService for proxy resolution.
-  // Further, if |alternative_proxy_server| is a valid but bad proxy, then
-  // fallback proxies are not used. It is illegal to call this with an
-  // initialized |alternative_service|, and a valid |alternative_proxy_server|.
-  Job(Delegate* delegate,
-      JobType job_type,
-      HttpNetworkSession* session,
-      const HttpRequestInfo& request_info,
-      RequestPriority priority,
-      const ProxyInfo& proxy_info,
-      const SSLConfig& server_ssl_config,
-      const SSLConfig& proxy_ssl_config,
-      HostPortPair destination,
-      GURL origin_url,
-      AlternativeService alternative_service,
+      NextProto alternative_protocol,
       const ProxyServer& alternative_proxy_server,
       bool enable_ip_based_pooling,
       NetLog* net_log);
@@ -231,10 +229,6 @@ class HttpStreamFactoryImpl::Job {
 
   JobType job_type() const { return job_type_; }
 
-  const AlternativeService alternative_service() const {
-    return alternative_service_;
-  }
-
   const ProxyServer alternative_proxy_server() const {
     return alternative_proxy_server_;
   }
@@ -334,10 +328,6 @@ class HttpStreamFactoryImpl::Job {
   // Set the motivation for this request onto the underlying socket.
   void SetSocketMotivation();
 
-  // Is this a SPDY or QUIC alternative Job?
-  bool IsSpdyAlternative() const;
-  bool IsQuicAlternative() const;
-
   // Sets several fields of |ssl_config| based on the proxy info and other
   // factors.
   void InitSSLConfig(SSLConfig* ssl_config, bool is_proxy) const;
@@ -346,8 +336,17 @@ class HttpStreamFactoryImpl::Job {
   // This must only be called when we are using an SSLSocket.
   void GetSSLInfo(SSLInfo* ssl_info);
 
+  // Called in Job constructor: should Job be forced to use QUIC.
+  static bool ShouldForceQuic(HttpNetworkSession* session,
+                              const HostPortPair& destination,
+                              GURL origin_url,

[xunjieli, 2017/06/14 18:28:01]

nit: const GURL& 

[Bence, 2017/06/14 19:33:30]

Done.

+                              const ProxyInfo& proxy_info);
+
   // Called in Job constructor. Use |spdy_session_key_| after construction.
-  SpdySessionKey GetSpdySessionKey() const;
+  static SpdySessionKey GetSpdySessionKey(bool spdy_session_direct,
+                                          const ProxyServer& proxy_server,
+                                          const GURL& origin_url,
+                                          PrivacyMode privacy_mode);
 
   // Returns true if the current request can use an existing spdy session.
   bool CanUseExistingSpdySession() const;
@@ -368,9 +367,6 @@ class HttpStreamFactoryImpl::Job {
   // Called to handle a client certificate request.
   int HandleCertificateRequest(int error);
 
-  // Should we force QUIC for this stream request.
-  bool ShouldForceQuic() const;
-
   ClientSocketPoolManager::SocketGroupType GetSocketGroup() const;
 
   void MaybeCopyConnectionAttemptsFromSocketOrHandle();
@@ -413,9 +409,6 @@ class HttpStreamFactoryImpl::Job {
   // original request when host mapping rules are set-up.
   const GURL origin_url_;
 
-  // AlternativeService for this Job if this is an alternative Job.
-  const AlternativeService alternative_service_;
-
   // Alternative proxy server that should be used by |this| to fetch the
   // request.
   const ProxyServer alternative_proxy_server_;
@@ -432,11 +425,16 @@ class HttpStreamFactoryImpl::Job {
   // True if handling a HTTPS request.
   const bool using_ssl_;
 
-  // True if this network transaction is using SPDY instead of HTTP.
-  bool using_spdy_;
+  // True if Job uses QUIC.
+  const bool using_quic_;
 
-  // True if this network transaction is using QUIC instead of HTTP.
-  bool using_quic_;
+  // True if Alternative Service protocol field requires that HTTP/2 is used.
+  // In this case, Job fails if it cannot pool to an existing SpdySession and
+  // the server does not negotiate HTTP/2 on a new socket.
+  const bool expect_spdy_;
+
+  // True if Job actually uses HTTP/2.
+  bool using_spdy_;
 
   // True if this job might succeed with a different proxy config.
   bool should_reconsider_proxy_;
@@ -517,7 +515,7 @@ class HttpStreamFactoryImpl::JobFactory {
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
-      AlternativeService alternative_service,
+      NextProto alternative_protocol,
       bool enable_ip_based_pooling,
       NetLog* net_log);