JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl_request.h

Index: net/http/http_stream_factory_impl_request.h
diff --git a/net/http/http_stream_factory_impl_request.h b/net/http/http_stream_factory_impl_request.h
index d63715631611ee8c7ca750e13cf5a16d446f8b02..c3bd87ec3b449f073d0880154bd2f2147bca9440 100644
--- a/net/http/http_stream_factory_impl_request.h
+++ b/net/http/http_stream_factory_impl_request.h
@@ -9,6 +9,7 @@
 #include <set>
 
 #include "base/macros.h"
+#include "net/base/net_export.h"
 #include "net/http/http_stream_factory_impl.h"
 #include "net/log/net_log.h"
 #include "net/socket/connection_attempts.h"
@@ -26,8 +27,25 @@ class SpdySession;
 
 class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
  public:
+  class NET_EXPORT_PRIVATE Helper {
+   public:
+    virtual ~Helper() {}
+
+    virtual LoadState GetLoadState() const = 0;
+
+    // Called when Request is destructed.
+    virtual void OnRequestComplete() = 0;
+
+    virtual int RestartTunnelWithProxyAuth(
+        const AuthCredentials& credentials) = 0;
+
+    virtual void SetPriority(RequestPriority priority) = 0;

[Ryan Hamilton, 2016/06/06 17:57:31]

nit: comment on these methods and on the class itself, please.

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+  };
+
+  // Request will notify |job_controller| when it's destructed.
+  // Thus |job_controller| is valid for the lifetime of the |this| Request.
   Request(const GURL& url,
-          HttpStreamFactoryImpl* factory,
+          Helper* helper,
           HttpStreamRequest::Delegate* delegate,
           WebSocketHandshakeStreamBase::CreateHelper*
               websocket_handshake_stream_create_helper,
@@ -41,39 +59,22 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
 
   const BoundNetLog& net_log() const { return net_log_; }
 
-  // Called when the Job determines the appropriate |spdy_session_key| for the
-  // Request. Note that this does not mean that SPDY is necessarily supported
-  // for this SpdySessionKey, since we may need to wait for NPN to complete
-  // before knowing if SPDY is available.
+  // Called when the |visitor_| determines the appropriate |spdy_session_key|

[Ryan Hamilton, 2016/06/06 17:57:31]

vistor or helper? (here and elsewhere)

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+  // for the Request. Note that this does not mean that SPDY is necessarily
+  // supported for this SpdySessionKey, since we may need to wait for NPN to
+  // complete before knowing if SPDY is available.
   void SetSpdySessionKey(const SpdySessionKey& spdy_session_key);
   bool HasSpdySessionKey() const;
 
-  // Attaches |job| to this request. Does not mean that Request will use |job|,
-  // but Request will own |job|.
-  void AttachJob(HttpStreamFactoryImpl::Job* job);
-
   // Marks completion of the request. Must be called before OnStreamReady().
   void Complete(bool was_npn_negotiated,
                 NextProto protocol_negotiated,
                 bool using_spdy);
 
-  // If this Request has a |spdy_session_key_|, remove this session from the
-  // SpdySessionRequestMap.
-  void RemoveRequestFromSpdySessionRequestMap();
-
-  // Called by an attached Job if it sets up a SpdySession.
-  // |stream| is null when |stream_type| is HttpStreamRequest::HTTP_STREAM.
-  // |bidirectional_stream_spdy_impl| is null when |stream_type| is
-  // HttpStreamRequest::BIDIRECTIONAL_STREAM.
-  void OnNewSpdySessionReady(
-      Job* job,
-      std::unique_ptr<HttpStream> stream,
-      std::unique_ptr<BidirectionalStreamImpl> bidirectional_stream_spdy_impl,
-      const base::WeakPtr<SpdySession>& spdy_session,
-      bool direct);
-
-  // Called by an attached Job to record connection attempts made by the socket
-  // layer for this stream request.
+  void ResetSpdySessionKey();
+
+  // Called by |visitor_| to record connection attempts made by the socket
+  // layer in an attached Job for this stream request.
   void AddConnectionAttempts(const ConnectionAttempts& attempts);
 
   WebSocketHandshakeStreamBase::CreateHelper*
@@ -84,37 +85,29 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
   // HttpStreamRequest::Delegate methods which we implement. Note we don't
   // actually subclass HttpStreamRequest::Delegate.
 
-  void OnStreamReady(Job* job,
-                     const SSLConfig& used_ssl_config,
+  void OnStreamReady(const SSLConfig& used_ssl_config,
                      const ProxyInfo& used_proxy_info,
                      HttpStream* stream);
-  void OnBidirectionalStreamImplReady(Job* job,
-                                      const SSLConfig& used_ssl_config,
+  void OnBidirectionalStreamImplReady(const SSLConfig& used_ssl_config,
                                       const ProxyInfo& used_proxy_info,
                                       BidirectionalStreamImpl* stream);
 
-  void OnWebSocketHandshakeStreamReady(Job* job,
-                                       const SSLConfig& used_ssl_config,
+  void OnWebSocketHandshakeStreamReady(const SSLConfig& used_ssl_config,
                                        const ProxyInfo& used_proxy_info,
                                        WebSocketHandshakeStreamBase* stream);
-  void OnStreamFailed(Job* job,
-                      int status,
+  void OnStreamFailed(int status,
                       const SSLConfig& used_ssl_config,
                       SSLFailureState ssl_failure_state);
-  void OnCertificateError(Job* job,
-                          int status,
+  void OnCertificateError(int status,
                           const SSLConfig& used_ssl_config,
                           const SSLInfo& ssl_info);
-  void OnNeedsProxyAuth(Job* job,
-                        const HttpResponseInfo& proxy_response,
+  void OnNeedsProxyAuth(const HttpResponseInfo& proxy_response,
                         const SSLConfig& used_ssl_config,
                         const ProxyInfo& used_proxy_info,
                         HttpAuthController* auth_controller);
-  void OnNeedsClientAuth(Job* job,
-                         const SSLConfig& used_ssl_config,
+  void OnNeedsClientAuth(const SSLConfig& used_ssl_config,
                          SSLCertRequestInfo* cert_info);
   void OnHttpsProxyTunnelResponse(
-      Job *job,
       const HttpResponseInfo& response_info,
       const SSLConfig& used_ssl_config,
       const ProxyInfo& used_proxy_info,
@@ -130,30 +123,22 @@ class HttpStreamFactoryImpl::Request : public HttpStreamRequest {
   bool using_spdy() const override;
   const ConnectionAttempts& connection_attempts() const override;
   HttpStreamRequest::StreamType stream_type() const { return stream_type_; }
+  const SpdySessionKey* spdy_session_key() const {
+    return spdy_session_key_.get();
+  }
 
  private:
-  // Used to bind |job| to the request and orphan all other jobs in |jobs_|.
-  void BindJob(Job* job);
-
-  // Used to orphan all jobs in |jobs_|.
-  void OrphanJobs();
-
-  // Used to cancel all jobs in |jobs_|.
-  void CancelJobs();
+  const GURL url_;
 
-  // Called when a Job succeeds.
-  void OnJobSucceeded(Job* job);
+  // Unowned.
+  // Safe pointer as |this| Request doesn't outlive the |helper_|.

[Ryan Hamilton, 2016/06/06 17:57:31]

I would write this as:

// Unowned. The helper must outlive this request.

[Zhongyi Shi, 2016/06/06 22:50:56]

Done.

+  Helper* helper_;
 
-  const GURL url_;
-  HttpStreamFactoryImpl* const factory_;
   WebSocketHandshakeStreamBase::CreateHelper* const
       websocket_handshake_stream_create_helper_;
   HttpStreamRequest::Delegate* const delegate_;
   const BoundNetLog net_log_;
 
-  // At the point where Job is irrevocably tied to the Request, we set this.
-  std::unique_ptr<Job> bound_job_;
-  std::set<HttpStreamFactoryImpl::Job*> jobs_;
   std::unique_ptr<const SpdySessionKey> spdy_session_key_;
 
   bool completed_;