JobController 1: Adding a new class HttpStreamFactoryImpl::JobController

net/http/http_stream_factory_impl.h

Index: net/http/http_stream_factory_impl.h
diff --git a/net/http/http_stream_factory_impl.h b/net/http/http_stream_factory_impl.h
index 4e20033bc20aa610ab8351eba8f00ad11867edb7..a92a028bcc7796d04d23f310784dcb43e969e7c2 100644
--- a/net/http/http_stream_factory_impl.h
+++ b/net/http/http_stream_factory_impl.h
@@ -25,6 +25,7 @@ namespace net {
 
 class HttpNetworkSession;
 class SpdySession;
+class TestJobControllerPeer;

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

I think this is unused?

 
 class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
  public:
@@ -62,17 +63,28 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   void PreconnectStreams(int num_streams, const HttpRequestInfo& info) override;
   const HostMappingRules* GetHostMappingRules() const override;
 
-  size_t num_orphaned_jobs() const { return orphaned_job_set_.size(); }
+  enum JobType {
+    MAIN,
+    ALTERNATIVE,
+    PRECONNECT,
+  };
 
  private:
   FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryImplRequestTest, SetPriority);
   FRIEND_TEST_ALL_PREFIXES(HttpStreamFactoryImplRequestTest, DelayMainJob);
 
+  friend class HttpStreamFactoryImplJobControllerTest;
+  friend class TestHttpStreamFactoryImplJob;
+  friend class TestJobFactory;

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

Does these need to be friends?

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

Unfortunately, yes. Those all access to HttpStreamFactoryImpl::Job which is a
private member of HttpStreamFactoryImpl, thus we have to list those classes as
friends. 

[Ryan Hamilton, 2016/06/13 19:25:26]

In that case, I wonder if we should make HttpStreamFactoryImpl::Job public. The
JobFactory needs to be able to use this class, obviously, which makes me think
public would be a good idea.

+
   class NET_EXPORT_PRIVATE Request;
   class NET_EXPORT_PRIVATE Job;
+  class NET_EXPORT_PRIVATE JobController;
+  class NET_EXPORT_PRIVATE JobFactoryImpl;

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

nit: I think we don't need NET_EXPORT_PRIVATE on any of these. Can you remove
them and see if it still works?

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

I can try after finish all the other part and sync to origin/master and do a git
cl try there then. 

 
   typedef std::set<Request*> RequestSet;
   typedef std::map<SpdySessionKey, RequestSet> SpdySessionRequestMap;
+  typedef std::set<std::unique_ptr<JobController>> JobControllerSet;
 
   HttpStreamRequest* RequestStreamInternal(
       const HttpRequestInfo& info,
@@ -84,14 +96,6 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
       HttpStreamRequest::StreamType stream_type,
       const BoundNetLog& net_log);
 
-  AlternativeService GetAlternativeServiceFor(
-      const HttpRequestInfo& request_info,
-      HttpStreamRequest::Delegate* delegate,
-      HttpStreamRequest::StreamType stream_type);
-
-  // Detaches |job| from |request|.
-  void OrphanJob(Job* job, const Request* request);
-
   // Called when a SpdySession is ready. It will find appropriate Requests and
   // fulfill them. |direct| indicates whether or not |spdy_session| uses a
   // proxy.
@@ -109,17 +113,12 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   // HttpAlternateProtocols with the failure and resets the SPDY session key.
   void OnBrokenAlternateProtocol(const Job*, const HostPortPair& origin);
 
-  // Invoked when an orphaned Job finishes.
-  void OnOrphanedJobComplete(const Job* job);
-
-  // Invoked when the Job finishes preconnecting sockets.
-  void OnPreconnectsComplete(const Job* job);
-
   // Called when the Preconnect completes. Used for testing.
   virtual void OnPreconnectsCompleteInternal() {}
 
-  // Returns true if QUIC is whitelisted for |host|.
-  bool IsQuicWhitelistedForHost(const std::string& host);
+  // Called when the JobController finishes service. Delete the JobController
+  // from |job_controller_set_|.
+  void OnJobControllerComplete(JobController* controller);
 
   HttpNetworkSession* const session_;
 
@@ -128,19 +127,16 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   // |request_map_|. The Requests will delete the corresponding job.
   std::map<const Job*, Request*> request_map_;
 
-  SpdySessionRequestMap spdy_session_request_map_;
+  // All Requests/Preconnects are assigned with a JobController to manage
+  // serving Job(s). JobController might outlive Request when Request
+  // is served while there's some working Job left. JobController will be
+  // deleted from |job_controller_set_| when it determines the completion of
+  // its work.
+  JobControllerSet job_controller_set_;
 
-  // These jobs correspond to jobs orphaned by Requests and now owned by
-  // HttpStreamFactoryImpl. Since they are no longer tied to Requests, they will
-  // not be canceled when Requests are canceled. Therefore, in
-  // ~HttpStreamFactoryImpl, it is possible for some jobs to still exist in this
-  // set. Leftover jobs will be deleted when the factory is destroyed.
-  std::set<const Job*> orphaned_job_set_;
-
-  // These jobs correspond to preconnect requests and have no associated Request
-  // object. They're owned by HttpStreamFactoryImpl. Leftover jobs will be
-  // deleted when the factory is destroyed.
-  std::set<const Job*> preconnect_job_set_;
+  std::unique_ptr<JobFactoryImpl> job_factory_;

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

nit: comment

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

Done.

+
+  SpdySessionRequestMap spdy_session_request_map_;
 
   const bool for_websockets_;
   DISALLOW_COPY_AND_ASSIGN(HttpStreamFactoryImpl);