Remove HttpStreamFactoryImpl::Job::MarkAsAlternate.

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 "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ skipping 18 matching lines @@
 class HttpAuthController;
 class HttpNetworkSession;
 class HttpStream;
 class SpdySessionPool;
 class QuicHttpStream;
 
 // An HttpStreamRequestImpl exists for each stream which is in progress of being
 // created for the StreamFactory.
 class HttpStreamFactoryImpl::Job {
  public:
+  // A Job is considered alternative iff

[Ryan Hamilton, 2015/06/02 17:26:56]

eschew "iff.  This reads just as unambiguiously when written as:

// A Job is considered alternative if 
// alternative_service.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL.

That being said, since this class doesn't really describe hat it means to be
"considered alternative" I would be more inclined to say something which simply
documents how this argument is used.

That being said, I think it might be clearer to have two different constructor
forms. One which takes an AlternateService and one which does not. You can then
use c++ 11's delegated constructor, I believe to make this simple.

[Bence, 2015/06/02 19:32:16]

Great idea, thanks.

Done.

+  // alternative_service.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL.
   Job(HttpStreamFactoryImpl* stream_factory,
       HttpNetworkSession* session,
       const HttpRequestInfo& request_info,
       RequestPriority priority,
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
+      AlternativeService alternative_service,
       NetLog* net_log);
   ~Job();
 
   // Start initiates the process of creating a new HttpStream. |request| will be
   // notified upon completion if the Job has not been Orphan()'d.
   void Start(Request* request);
 
   // Preconnect will attempt to request |num_streams| sockets from the
   // appropriate ClientSocketPool.
   int Preconnect(int num_streams);
 
   int RestartTunnelWithProxyAuth(const AuthCredentials& credentials);
   LoadState GetLoadState() const;
 
-  // Marks this Job as the "alternate" job, from Alternate-Protocol or Alt-Svc
-  // using the specified alternate service.
-  void MarkAsAlternate(AlternativeService alternative_service);
-
   // Tells |this| to wait for |job| to resume it.
   void WaitFor(Job* job);
 
   // Tells |this| that |job| has determined it still needs to continue
   // connecting, so allow |this| to continue. If this is not called, then
   // |request_| is expected to cancel |this| by deleting it.
   void Resume(Job* job);
 
   // Used to detach the Job from |request|.
   void Orphan(const Request* request);
@@ skipping 66 matching lines @@
     STATUS_BROKEN,
     STATUS_SUCCEEDED
   };
 
   // Wrapper class for SpdySessionPool methods to enforce certificate
   // requirements for SpdySessions.
   class ValidSpdySessionPool {
    public:
     ValidSpdySessionPool(SpdySessionPool* spdy_session_pool,
                          GURL& origin_url,
-                         bool is_spdy_alternate);
+                         bool is_spdy_alternative);
 
     // Returns OK if a SpdySession was not found (in which case |spdy_session|
     // is set to nullptr), or if one was found (in which case |spdy_session| is
     // set to it) and it has an associated SSL certificate with is valid for
     // |origin_url_|, or if this requirement does not apply because the Job is
-    // not a SPDY alternate job.  Returns the appropriate error code otherwise,
+    // not a SPDY alternative job.  Returns the appropriate error code
+    // otherwise,
     // in which case |spdy_session| should not be used.
     int FindAvailableSession(const SpdySessionKey& key,
                              const BoundNetLog& net_log,
                              base::WeakPtr<SpdySession>* spdy_session);
 
     // Creates a SpdySession and sets |spdy_session| to point to it.  Returns OK
     // if the associated SSL certificate is valid for |origin_url_|, or if this
-    // requirement does not apply because the Job is not a SPDY alternate job.
+    // requirement does not apply because the Job is not a SPDY alternative job.
     // Returns the appropriate error code otherwise, in which case
     // |spdy_session| should not be used.
     int CreateAvailableSessionFromSocket(
         const SpdySessionKey& key,
         scoped_ptr<ClientSocketHandle> connection,
         const BoundNetLog& net_log,
         int certificate_error_code,
         bool is_secure,
         base::WeakPtr<SpdySession>* spdy_session);
 
    private:
     // Returns OK if |spdy_session| has an associated SSL certificate with is
     // valid for |origin_url_|, or if this requirement does not apply because
-    // the Job is not a SPDY alternate job, or if |spdy_session| is null.
+    // the Job is not a SPDY alternative job, or if |spdy_session| is null.
     // Returns appropriate error code otherwise.
     int CheckAlternativeServiceValidityForOrigin(
         base::WeakPtr<SpdySession> spdy_session);
 
     SpdySessionPool* const spdy_session_pool_;
     const GURL origin_url_;
-    const bool is_spdy_alternate_;
+    const bool is_spdy_alternative_;
   };
 
   void OnStreamReadyCallback();
   void OnWebSocketHandshakeStreamReadyCallback();
   // This callback function is called when a new SPDY session is created.
   void OnNewSpdySessionReadyCallback();
   void OnStreamFailedCallback(int result);
   void OnCertificateErrorCallback(int result, const SSLInfo& ssl_info);
   void OnNeedsProxyAuthCallback(const HttpResponseInfo& response_info,
                                 HttpAuthController* auth_controller);
@@ skipping 29 matching lines @@
   int SetSpdyHttpStream(base::WeakPtr<SpdySession> session, bool direct);
 
   // Returns to STATE_INIT_CONNECTION and resets some state.
   void ReturnToStateInitConnection(bool close_connection);
 
   // Set the motivation for this request onto the underlying socket.
   void SetSocketMotivation();
 
   bool IsHttpsProxyAndHttpUrl() const;
 
-  // Returns true iff this Job is an alternate, that is, iff MarkAsAlternate has
-  // been called.
-  bool IsAlternate() const;
-
-  // Returns true if this Job is a SPDY alternate job.
-  bool IsSpdyAlternate() const;
-
   // Sets several fields of |ssl_config| for |server| based on the proxy info
   // and other factors.
   void InitSSLConfig(const HostPortPair& server,
                      SSLConfig* ssl_config,
                      bool is_proxy) const;
 
   // Retrieve SSLInfo from our SSL Socket.
   // This must only be called when we are using an SSLSocket.
   // After calling, the caller can use ssl_info_.
   void GetSSLInfo();
@@ skipping 61 matching lines @@
   SSLInfo ssl_info_;
 
   // The server we are trying to reach, could be that of the origin or of the
   // alternative service.
   HostPortPair server_;
 
   // The origin url we're trying to reach. This url may be different from the
   // original request when host mapping rules are set-up.
   GURL origin_url_;
 
-  // AlternateProtocol for this job if this is an alternate job.
-  AlternativeService alternative_service_;
+  // AlternativeService for this Job if this is an alternative Job.
+  const AlternativeService alternative_service_;
 
-  // AlternateProtocol for the other job if this is not an alternate job.
+  // Is this a SPDY or QUIC alternative Job?
+  const bool is_spdy_alternative_;
+  const bool is_quic_alternative_;

[Ryan Hamilton, 2015/06/02 17:26:57]

It's not obvious to me that this is a win, but perhaps it is. So this adds to
the memory used by each job (albeit not very much) and denormalizes information
out of alternative_service_ into these two new members. There's nothing *wrong*
with this of course, but I don't know that it's better?

[Bence, 2015/06/02 19:32:16]

I agree: it doesn't seem to matter much.  I reverted to methods for lack of
compelling reason to use members.

Done.

+
+  // AlternativeService for the other Job if this is not an alternative Job.
   AlternativeService other_job_alternative_service_;
 
   // This is the Job we're dependent on. It will notify us if/when it's OK to
   // proceed.
   Job* blocking_job_;
 
   // |waiting_job_| is a Job waiting to see if |this| can reuse a connection.
   // If |this| is unable to do so, we'll notify |waiting_job_| that it's ok to
   // proceed and then race the two Jobs.
   Job* waiting_job_;
@@ skipping 52 matching lines @@
   JobStatus other_job_status_;
 
   base::WeakPtrFactory<Job> ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_H_