Extract Proxy Resolution out of HttpStreamFactoryImpl::Job

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 809574ed30b4756c29679858b5e45608f4e30b18..6ac1a75df9ed0204cc9433fc40e49ad40c40d129 100644
--- a/net/http/http_stream_factory_impl_job.h
+++ b/net/http/http_stream_factory_impl_job.h
@@ -102,16 +102,6 @@ class HttpStreamFactoryImpl::Job {
     // contained in |proxy_info| can be skipped.
     virtual bool OnInitConnection(const ProxyInfo& proxy_info) = 0;
 
-    // Invoked when |job| has completed proxy resolution. The delegate may
-    // create an alternative proxy server job to fetch the request.
-    virtual void OnResolveProxyComplete(
-        Job* job,
-        const HttpRequestInfo& request_info,
-        RequestPriority priority,
-        const SSLConfig& server_ssl_config,
-        const SSLConfig& proxy_ssl_config,
-        HttpStreamRequest::StreamType stream_type) = 0;
-
     // Invoked to notify the Request and Factory of the readiness of new
     // SPDY session.
     virtual void OnNewSpdySessionReady(
@@ -163,6 +153,7 @@ class HttpStreamFactoryImpl::Job {
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
+      ProxyInfo proxy_info,

[tbansal1, 2017/05/09 20:38:48]

pass as const ref?
here and in the ctor below.

[xunjieli, 2017/05/10 00:26:24]

Done.

       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
@@ -183,6 +174,7 @@ class HttpStreamFactoryImpl::Job {
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
+      ProxyInfo proxy_info,
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
@@ -251,6 +243,8 @@ class HttpStreamFactoryImpl::Job {
     return using_existing_quic_session_;
   }
 
+  bool reconsider_proxy() const { return reconsider_proxy_; }
+
   // TODO(xunjieli): Added to investigate crbug.com/711721. Remove when no
   // longer needed.
   void LogHistograms() const;
@@ -260,9 +254,6 @@ class HttpStreamFactoryImpl::Job {
 
   enum State {
     STATE_START,
-    STATE_RESOLVE_PROXY,
-    STATE_RESOLVE_PROXY_COMPLETE,
-
     // The main and alternative jobs are started in parallel.  The main job
     // waits after it finishes proxy resolution.  The alternative job never
     // waits.
@@ -321,8 +312,6 @@ class HttpStreamFactoryImpl::Job {
   // ERR_IO_PENDING, then the result from OnIOComplete will be passed to the
   // next state method as the result arg.
   int DoStart();
-  int DoResolveProxy();
-  int DoResolveProxyComplete(int result);
   int DoWait();
   int DoWaitComplete(int result);
   int DoInitConnection();
@@ -405,7 +394,7 @@ class HttpStreamFactoryImpl::Job {
 
   const HttpRequestInfo request_info_;
   RequestPriority priority_;
-  ProxyInfo proxy_info_;
+  const ProxyInfo proxy_info_;
   SSLConfig server_ssl_config_;
   SSLConfig proxy_ssl_config_;
   const NetLogWithSource net_log_;
@@ -418,7 +407,6 @@ class HttpStreamFactoryImpl::Job {
   State state_;
 
   State next_state_;
-  ProxyService::PacRequest* pac_request_;
   SSLInfo ssl_info_;
 
   // The server we are trying to reach, could be that of the origin or of the
@@ -453,6 +441,10 @@ class HttpStreamFactoryImpl::Job {
 
   // True if this network transaction is using QUIC instead of HTTP.
   bool using_quic_;
+
+  // True if this job might succeed with a different proxy config.
+  bool reconsider_proxy_;

[Ryan Hamilton, 2017/05/09 19:22:06]

nit: Consider "should_reconsider_proxy_"

[xunjieli, 2017/05/10 00:26:24]

Done.

+
   QuicStreamRequest quic_request_;
 
   // True if this job used an existing QUIC session.
@@ -498,51 +490,53 @@ class HttpStreamFactoryImpl::Job {
 // Factory for creating Jobs.
 class HttpStreamFactoryImpl::JobFactory {
  public:
-  virtual ~JobFactory() {}
+  JobFactory();
+
+  virtual ~JobFactory();
 
-  // Creates an alternative service Job.
   virtual HttpStreamFactoryImpl::Job* CreateJob(

[Ryan Hamilton, 2017/05/09 19:22:06]

nit: There are quite a few arguments in each of these 3 methods which makes it a
bit non-obvious to tell the difference. Could you add short comments before each
which describes how they're different?

Or even better, consider renaming them to be a bit more descriptive?

CreateMainJob
CreateAlternativeJob
CreateAlternativeProxyJob

(Though maybe that'd be better in cleanup/followup)

[xunjieli, 2017/05/10 00:26:24]

Done.

       HttpStreamFactoryImpl::Job::Delegate* delegate,
       HttpStreamFactoryImpl::JobType job_type,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
+      ProxyInfo proxy_info,

[tbansal1, 2017/05/09 20:38:48]

nit: pass as const ref? here and below.

[xunjieli, 2017/05/10 00:26:24]

Done.

       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
-      AlternativeService alternative_service,
       bool enable_ip_based_pooling,
-      NetLog* net_log) = 0;
+      NetLog* net_log);
 
-  // Creates an alternative proxy server Job.
   virtual HttpStreamFactoryImpl::Job* CreateJob(
       HttpStreamFactoryImpl::Job::Delegate* delegate,
       HttpStreamFactoryImpl::JobType job_type,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
+      ProxyInfo proxy_info,
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
-      const ProxyServer& alternative_proxy_server,
+      AlternativeService alternative_service,
       bool enable_ip_based_pooling,
-      NetLog* net_log) = 0;
+      NetLog* net_log);
 
-  // Creates a non-alternative Job.
   virtual HttpStreamFactoryImpl::Job* CreateJob(
       HttpStreamFactoryImpl::Job::Delegate* delegate,
       HttpStreamFactoryImpl::JobType job_type,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
+      ProxyInfo proxy_info,
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
+      const ProxyServer& alternative_proxy_server,
       bool enable_ip_based_pooling,
-      NetLog* net_log) = 0;
+      NetLog* net_log);
 };
 
 }  // namespace net