Extract Proxy Resolution out of HttpStreamFactoryImpl::Job

net/http/http_stream_factory_impl_job.h

 // 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.
 
 #ifndef NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_
 #define NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_
 
 #include <memory>
 #include <utility>
 
@@ skipping 84 matching lines @@
     virtual void OnNeedsProxyAuth(Job* job,
                                   const HttpResponseInfo& proxy_response,
                                   const SSLConfig& used_ssl_config,
                                   const ProxyInfo& used_proxy_info,
                                   HttpAuthController* auth_controller) = 0;
 
     // Returns true if the connection initialization to the proxy server
     // 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(
         Job* job,
         const base::WeakPtr<SpdySession>& spdy_session,
         bool direct) = 0;
 
     // Invoked when the |job| finishes pre-connecting sockets.
     virtual void OnPreconnectsComplete(Job* job) = 0;
 
@@ skipping 31 matching lines @@
   };
 
   // Constructor for non-alternative Job.
   // Job is owned by |delegate|, hence |delegate| is valid for the
   // lifetime of the Job.
   Job(Delegate* delegate,
       JobType job_type,
       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,
       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,
+      ProxyInfo proxy_info,
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HostPortPair destination,
       GURL origin_url,
       AlternativeService alternative_service,
       const ProxyServer& alternative_proxy_server,
       bool enable_ip_based_pooling,
       NetLog* net_log);
   virtual ~Job();
 
@@ skipping 48 matching lines @@
   }
 
   const ProxyServer alternative_proxy_server() const {
     return alternative_proxy_server_;
   }
 
   bool using_existing_quic_session() const {
     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;
 
  private:
   friend class HttpStreamFactoryImplJobPeer;
 
   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.
     //
     // An HTTP/2 alternative job notifies the JobController in DoInitConnection
     // unless it can pool to an existing SpdySession.  JobController, in turn,
     // resumes the main job.
     //
     // A QUIC alternative job notifies the JobController in DoInitConnection
     // regardless of whether it pools to an existing QUIC session, but the main
@@ skipping 38 matching lines @@
   void RunLoop(int result);
   int DoLoop(int result);
   int StartInternal();
   int DoInitConnectionImpl();
 
   // Each of these methods corresponds to a State value.  Those with an input
   // argument receive the result from the previous state.  If a method returns
   // 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();
   int DoInitConnectionComplete(int result);
   int DoWaitingUserAction(int result);
   int DoCreateStream();
   int DoCreateStreamComplete(int result);
   int DoRestartTunnelAuth();
   int DoRestartTunnelAuthComplete(int result);
 
@@ skipping 62 matching lines @@
   // session is found, and OK otherwise.
   static int OnHostResolution(SpdySessionPool* spdy_session_pool,
                               const SpdySessionKey& spdy_session_key,
                               const GURL& origin_url,
                               bool enable_ip_based_pooling,
                               const AddressList& addresses,
                               const NetLogWithSource& net_log);
 
   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_;
 
   CompletionCallback io_callback_;
   std::unique_ptr<ClientSocketHandle> connection_;
   HttpNetworkSession* const session_;
 
   // |state_| is only used for LogHistograms().
   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
   // alternative service (after applying host mapping rules).
   const HostPortPair destination_;
 
   // The origin url we're trying to reach. This url may be different from the
   // original request when host mapping rules are set-up.
   const GURL origin_url_;
 
@@ skipping 14 matching lines @@
   const JobType job_type_;
 
   // 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 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.
   bool using_existing_quic_session_;
 
   // True when the tunnel is in the process of being established - we can't
   // read from the socket until the tunnel is done.
   bool establishing_tunnel_;
 
   std::unique_ptr<HttpStream> stream_;
@@ skipping 25 matching lines @@
   HttpStreamRequest::StreamType stream_type_;
 
   base::WeakPtrFactory<Job> ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 // Factory for creating Jobs.
 class HttpStreamFactoryImpl::JobFactory {
  public:
-  virtual ~JobFactory() {}
+  JobFactory();
 
-  // Creates an alternative service Job.
+  virtual ~JobFactory();
+
   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,
+      bool enable_ip_based_pooling,
+      NetLog* net_log);
+
+  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,
       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,
       bool enable_ip_based_pooling,
-      NetLog* net_log) = 0;
-
-  // 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,
-      const SSLConfig& server_ssl_config,
-      const SSLConfig& proxy_ssl_config,
-      HostPortPair destination,
-      GURL origin_url,
-      bool enable_ip_based_pooling,
-      NetLog* net_log) = 0;
+      NetLog* net_log);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_