Respect QuicAllowed policy for new streams

net/http/http_network_session.h

Index: net/http/http_network_session.h
diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h
index f1402bfae2908ed943c496809d2ab0037e74fdcb..f14501f2060c286cd888f969fca479a613abf7c5 100644
--- a/net/http/http_network_session.h
+++ b/net/http/http_network_session.h
@@ -70,6 +70,20 @@ class NET_EXPORT HttpNetworkSession
     : NON_EXPORTED_BASE(public base::NonThreadSafe),
       public base::MemoryCoordinatorClient {
  public:
+  // Shared Dynamic Parameters - holds parameters which can change at run time.
+  // One instance of this struct is intended to be shared accross multiple
+  // HttpNetworkSessions (the default copy constructor of Params just copies the
+  // pointer to SharedParams).
+  struct NET_EXPORT DynamicSharedParams {
+    DynamicSharedParams();
+    DynamicSharedParams(const DynamicSharedParams& other);
+    ~DynamicSharedParams();
+
+    // Enables QUIC for new streams (if QUIC is enabled).
+    // If DynamicSharedParams is missing, assume false.
+    bool enable_quic;

[Ryan Hamilton, 2016/12/20 15:38:12]

nit: quic_enabled might be a better choice since it sounds more like a state
instead of like an action.

[pmarko, 2016/12/20 18:00:37]

I also thought so at first, but then I wanted to make this consistent with
Params::enable_* - seeing as this used to be Params::enable_quic.

What would you prefer -- consistency with Params:: members or the higher clarity
of action/state semantics?

[Ryan Hamilton, 2016/12/21 21:23:58]

Ah, fair enough. Let's remain consistent.

[pmarko, 2016/12/23 19:19:46]

(obsolete - Params has been reverted)

+  };
+
   struct NET_EXPORT Params {
     Params();
     Params(const Params& other);
@@ -94,6 +108,12 @@ class NET_EXPORT HttpNetworkSession
     uint16_t testing_fixed_https_port;
     bool enable_tcp_fast_open_for_ssl;
 
+    // Holds parameters which can change after initialization
+    // and can be shared accross multiple HttpNetworkSessions.
+    // By default, the pointer is inherited on copy.
+    // May be nullptr

[Bence, 2016/12/20 14:58:07]

Please append period to last sentence.

[Ryan Hamilton, 2016/12/20 15:38:12]

I'm ramping up on this CL, so sorry for the basic question. Under what
circumstances should this be null?

[pmarko, 2016/12/20 18:00:37]

Bence: Done.

Ryan: Good point! Basically, by introducing this, I've added a burden of
creating and assigning the dynamic_shared_params when using HttpNetworkSession
with QUIC. As I didn't want to touch all existing unit tests which create a
HttpNetworkSession, I've thought of this:
- When dynamic_shared_params==nullptr, use what used to be default if the user
code didn't change enable_quic.
- This means that existing unit tests which don't use QUIC need no changes (this
remains nullptr, and the resulting behavior is enable_quic=false, which is the
same as they had until now).

I've added a comment clarifying this and referencing to comments on
DynamicSharedParams:: members, where the defaults are commented.

[Ryan Hamilton, 2016/12/21 21:23:58]

Hm. I think you're saying that the dynamic params should never be null in
production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params !=
null); in HttpNetworkSession to make sure callers do the right thing. Alas, as
you point out, this means that a bunch of tests will need to change. That's
annoying, but less annoying than accidentally not enabling QUIC.

[pmarko, 2016/12/23 19:19:46]

See my reply in comments thread - you're right that this is not really
consitent, and making it consistent and thus requiring everyone to set this is
too big a change for what I'm trying to achieve in my opinion.

Thus: Changed the logic: No more DynamicSharedParams, instead HttpNetworkSession
now has SetQuicEnabled which is called from outside.

+    DynamicSharedParams* dynamic_shared_params;

[Ryan Hamilton, 2016/12/20 15:38:12]

I'm surprised not to see any cronet code which sets this. Can you confirm that
cronet's QUIC-enabling code still works after this refactor?

[pmarko, 2016/12/20 18:00:37]

Good point! Didn't think of cronet, but it should be fine - I've just checked
that cronet_environment.mm uses net::URLRequestContextBuilder which sets this
(see cronet_environment.mm:295 , url_request_context_config.cc:373, and
url_request_context_builder.cc in this patch).

[Ryan Hamilton, 2016/12/21 21:23:58]

Ah, good point.

[pmarko, 2016/12/23 19:19:46]

(obsolete - Params has been reverted)

+
     // Use SPDY ping frames to test for connection health after idle.
     bool enable_spdy_ping_based_connection_checking;
     bool enable_http2;
@@ -108,8 +128,6 @@ class NET_EXPORT HttpNetworkSession
     // of the origin.
     bool enable_quic_alternative_service_with_different_host;
 
-    // Enables QUIC support.
-    bool enable_quic;
     // Disable QUIC if a connection times out with open streams.
     bool disable_quic_on_timeout_with_open_streams;
     // Disables QUIC's 0-RTT behavior.
@@ -194,6 +212,9 @@ class NET_EXPORT HttpNetworkSession
     // Enable HTTP/0.9 for HTTP/HTTPS on ports other than the default one for
     // each protocol.
     bool http_09_on_non_default_ports_enabled;
+
+    // Evaluates if QUIC is enabled for new streams

[Bence, 2016/12/20 14:58:07]

Please append period to this sentence.

[pmarko, 2016/12/20 18:00:37]

Done.

+    bool enable_quic() const;

[Bence, 2016/12/20 14:58:07]

Please move this up to right after the destructor: methods (even accessors)
usually come before members, see
https://google.github.io/styleguide/cppguide.html#Declaration_Order.

[Ryan Hamilton, 2016/12/20 15:38:12]

The style guide prohibits methods on structs. Perhaps you could add an
IsQuicEnabled() method to the session itself?

[pmarko, 2016/12/20 18:00:37]

Bence: Probably obselete now :)

Ryan: Ah, I didn't know that, but it makes sense. Will do.

   };
 
   enum SocketPoolType {