Remove HttpStreamFactory's NPN/SPDY globals, except for spdy_enabled.

Remove HttpStreamFactory's NPN/SPDY globals, except for spdy_enabled.

Instead, each HttpNetworkSession is given its own immutable copies on
construction.  Other than spdy_enabled, none of the globals were
changed before this CL, anyways.

Also, setting spdy_enabled back to true after setting it to false no
longer clears the NPN list.

spdy_enabled is still a global because group policy can set it to
false at runtime.

BUG=372533
R=joaodasilva@chromium.org, rch@chromium.org, sergeyu@chromium.org, sgurun@chromium.org, sky@chromium.org, ttuttle@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272698

[mmenke, 2014-05-22 14:51:12]

Ryan:  I've tried to keep code changes to a minimum in this CL, beyond moving
stuff into the session and making it still possible to enable/disable SPDY at
runtime (Group policy still relies on that behavior, no idea how important
supporting updates there at runtime is, but think it belongs in its own CL, if
we do that).

There are certainly cleanups worth considering in followup CLs, like getting rid
of |enabled_protocols_|, and if we really need use_alternate_protocols, instead
of just setting next_protos to be empty, or include only HTTP.

https://codereview.chromium.org/284423002/diff/330001/components/cronet/andro...
File components/cronet/android/url_request_context_peer.cc (right):

https://codereview.chromium.org/284423002/diff/330001/components/cronet/andro...
components/cronet/android/url_request_context_peer.cc:135: // TODO(mmenke):  Add
method to have the builder enable SPDY.
I plan to do this right after this CL lands.

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:285: }
Note:  This is a behavior change:  It used to be when next_protos_ was NULL, we
did nothing.  This is not the same as spdy_enabled being false, and it doesn't
look to me like we ever set the SSL config's next protos, anyways.

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:285: }
Another behavior change:  If spdy is disabled and then enabled, we effectively
restore next_protos.  I suspect this is the behavior we want, anyways, though it
presumably doesn't happen often.

[sgurun-gerrit only, 2014-05-22 15:49:52]

On 2014/05/22 14:51:12, mmenke wrote:
> Ryan:  I've tried to keep code changes to a minimum in this CL, beyond moving
> stuff into the session and making it still possible to enable/disable SPDY at
> runtime (Group policy still relies on that behavior, no idea how important
> supporting updates there at runtime is, but think it belongs in its own CL, if
> we do that).
> 
> There are certainly cleanups worth considering in followup CLs, like getting
rid
> of |enabled_protocols_|, and if we really need use_alternate_protocols,
instead
> of just setting next_protos to be empty, or include only HTTP.
> 
>
https://codereview.chromium.org/284423002/diff/330001/components/cronet/andro...
> File components/cronet/android/url_request_context_peer.cc (right):
> 
>
https://codereview.chromium.org/284423002/diff/330001/components/cronet/andro...
> components/cronet/android/url_request_context_peer.cc:135: // TODO(mmenke): 
Add
> method to have the builder enable SPDY.
> I plan to do this right after this CL lands.
> 
>
https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
> File net/http/http_network_session.cc (right):
> 
>
https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
> net/http/http_network_session.cc:285: }
> Note:  This is a behavior change:  It used to be when next_protos_ was NULL,
we
> did nothing.  This is not the same as spdy_enabled being false, and it doesn't
> look to me like we ever set the SSL config's next protos, anyways.
> 
>
https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
> net/http/http_network_session.cc:285: }
> Another behavior change:  If spdy is disabled and then enabled, we effectively
> restore next_protos.  I suspect this is the behavior we want, anyways, though
it
> presumably doesn't happen often.

aw lgtm

[Ryan Hamilton, 2014-05-22 17:30:45]

LGTM. Looks great. It's too bad the spdy_enabled thing needs to stay static, but
that's not too bad! Nicely done.

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:152: for (int i =
ALTERNATE_PROTOCOL_MINIMUM_VALID_VERSION;
nit: I think you'll need to use size_t here (or some unsigned type) or else you
get a compiler error on some platforms about the signed/unsigned comparison on
the next line.

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:285: }
On 2014/05/22 14:51:12, mmenke wrote:
> Another behavior change:  If spdy is disabled and then enabled, we effectively
> restore next_protos.  I suspect this is the behavior we want, anyways, though
it
> presumably doesn't happen often.

Oh, I see. That explains why this method doesn't simply return next_protos_;

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.h:207: void
GetNextProtos(std::vector<std::string>* next_protos) const;
I would have expected this method to return a const std::vector& instead of
using an outparam. Would that be feasible?

[mmenke, 2014-05-22 20:12:16]

Thanks for the feedback!

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:152: for (int i =
ALTERNATE_PROTOCOL_MINIMUM_VALID_VERSION;
On 2014/05/22 17:30:45, Ryan Hamilton wrote:
> nit: I think you'll need to use size_t here (or some unsigned type) or else
you
> get a compiler error on some platforms about the signed/unsigned comparison on
> the next line.

I'm pretty sure enums are signed on all platforms.  This was actually just
copied from the old code, without modification, so presumably it compiles
everywhere.

https://codereview.chromium.org/284423002/diff/330001/net/http/http_network_s...
net/http/http_network_session.cc:285: }
On 2014/05/22 17:30:45, Ryan Hamilton wrote:
> On 2014/05/22 14:51:12, mmenke wrote:
> > Another behavior change:  If spdy is disabled and then enabled, we
effectively
> > restore next_protos.  I suspect this is the behavior we want, anyways,
though
> it
> > presumably doesn't happen often.
> 
> Oh, I see. That explains why this method doesn't simply return next_protos_;

Yea, I thought this option was cleaner than having all callsites check if it
were "NULL" or not.  If you prefer the old pattern, or have another suggestion,
I'm certainly open to changing things.

[mmenke, 2014-05-22 20:40:35]

[+sky]:  Mind signing off on the io_thread changes?

[+sanjeevr]:  Please review jingle/glue (Just copying network params that are no
longer globals).

[+ttuttle]:  Please review components/domain_reliability (Fix minor
include-what-you-use issue, I'll just TBR you if I hear back from the others
first)

[mmenke, 2014-05-22 20:40:35]

[+sky]:  Mind signing off on the io_thread changes?

[+sanjeevr]:  Please review jingle/glue (Just copying network params that are no
longer globals).

[+ttuttle]:  Please review components/domain_reliability (Fix minor
include-what-you-use issue, I'll just TBR you if I hear back from the others
first)

[mmenke, 2014-05-22 20:41:40]

[+joaodasilva]:  This also fixes an include-what-you-use issue in
chrome/browser/policy/url_blacklist_manager_unittest.cc.  If you want to review,
feel free.  Otherwise, I'll TBR you.

[sky, 2014-05-22 21:43:17]

LGTM

[Deprecated (see juliatuttle), 2014-05-22 22:58:56]

On 2014/05/22 20:40:35, mmenke wrote:
> [+ttuttle]:  Please review components/domain_reliability (Fix minor
> include-what-you-use issue, I'll just TBR you if I hear back from the others
> first)

lgtm.

[Joao da Silva, 2014-05-23 06:48:48]

policy lgtm

[mmenke, 2014-05-23 14:31:22]

[+sergeyu, -sanjeevr]:  Please review
jingle/glue/proxy_resolving_client_socket.cc.  Should sanjeevr be removed from
jingle's OWNERS file?  Looks like sanjeevr's last commit was in 2012, and seems
like email addresses that essentially blackhole review requests shouldn't still
be listed.

[mmenke, 2014-05-23 14:32:59]

On 2014/05/23 14:31:22, mmenke wrote:
> [+sergeyu, -sanjeevr]:  Please review
> jingle/glue/proxy_resolving_client_socket.cc.  Should sanjeevr be removed from
> jingle's OWNERS file?  Looks like sanjeevr's last commit was in 2012, and
seems
> like email addresses that essentially blackhole review requests shouldn't
still
> be listed.

(Oh, and I should note that I waited less than a day before checking sanjeevr's
status, so it's entirely possible he still actively does reviews, but I'm
assuming that's not the case)

[Sergey Ulanov, 2014-05-23 22:02:20]

LGTM

https://codereview.chromium.org/284423002/diff/240029/jingle/glue/proxy_resol...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/284423002/diff/240029/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:74: // prone.  Should have a better
way to do this.
Perhaps session_params should be initialized as a copy of reference_params and
then all other parameters can be set as necessary (lines 54-68)

[Sergey Ulanov, 2014-05-23 22:03:21]

On 2014/05/23 14:31:22, mmenke wrote:
> [+sergeyu, -sanjeevr]:  Please review
> jingle/glue/proxy_resolving_client_socket.cc.  Should sanjeevr be removed from
> jingle's OWNERS file?  Looks like sanjeevr's last commit was in 2012, and
seems
> like email addresses that essentially blackhole review requests shouldn't
still
> be listed.

Yes, Sanjeev hasn't been active for awhile. I'll remove him from OWNERS.

[mmenke, 2014-05-23 22:06:57]

Thanks for the reviews!

https://codereview.chromium.org/284423002/diff/240029/jingle/glue/proxy_resol...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/284423002/diff/240029/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:74: // prone.  Should have a better
way to do this.
On 2014/05/23 22:02:21, Sergey Ulanov wrote:
> Perhaps session_params should be initialized as a copy of reference_params and
> then all other parameters can be set as necessary (lines 54-68)

Yea, the params do look to be copyable.  I'll investigate in a followup CL.

[mmenke, 2014-05-23 22:07:04]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-05-23 22:09:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/284423002/240029

[mmenke, 2014-05-23 23:15:31]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-05-23 23:17:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/284423002/350001

[mmenke, 2014-05-23 23:46:27]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-05-23 23:49:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/284423002/370001

[mmenke, 2014-05-24 03:38:46]

Committed patchset #7 manually as r272698 (presubmit successful).

[Ryan Hamilton, 2014-07-02 19:33:10]

Curiously, this seems to have converted http_stream_factory to DOS line endings.
 I'll attempt to fix this in https://codereview.chromium.org/365973004/