Get rid of websocket_over_spdy_enabled global.

Get rid of websocket_over_spdy_enabled global.

It's now called enable_websocket_over_spdy and is a member of
HttpNetworkSession::Params.

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

[mmenke, 2014-05-29 17:32:20]

rch:  Please review everything but the websockets code.
ricea:  Please review websockets code and chrome code (I'll get an owner of
io_thread after I get you two to signoff).

[Ryan Hamilton, 2014-05-29 19:26:50]

LGTM, modulo a few nits. Thanks for doing this!

I also think you need to s/not/now/ in the CL description.

https://codereview.chromium.org/295383007/diff/10011/net/http/http_network_se...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/295383007/diff/10011/net/http/http_network_se...
net/http/http_network_session.h:122: bool enable_websocket_over_spdy;
Looks like there is not an empty line before the other members.  When in Rome...

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
File net/websockets/websocket_job_test.cc (right):

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
net/websockets/websocket_job_test.cc:277: MockHttpTransactionFactory(NextProto
next_proto, OrderedSocketData* data,
nit: 1 per line, since it no longer fits on one line.

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
net/websockets/websocket_job_test.cc:394: WebSocketJobTest() :
spdy_util_(GetParam()),
I think you need to start the initalizer list on a new line, since it doesn't
fit on a single line.

[mmenke, 2014-05-29 19:53:13]

Thanks for the feedback!

https://codereview.chromium.org/295383007/diff/10011/net/http/http_network_se...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/295383007/diff/10011/net/http/http_network_se...
net/http/http_network_session.h:122: bool enable_websocket_over_spdy;
On 2014/05/29 19:26:50, Ryan Hamilton wrote:
> Looks like there is not an empty line before the other members.  When in
Rome...

Done, though not sure I agree - SPDY and QUIC settings are separated out, and I
consider this a start of a new (And hopefully never to grow) WebSocket section.

Regardless, not something I feel strongly about.

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
File net/websockets/websocket_job_test.cc (right):

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
net/websockets/websocket_job_test.cc:394: WebSocketJobTest() :
spdy_util_(GetParam()),
On 2014/05/29 19:26:50, Ryan Hamilton wrote:
> I think you need to start the initalizer list on a new line, since it doesn't
> fit on a single line.

Done.  Looks like you're right.  Rule is often violated, and few enough
reviewers enforce it so I haven't internalized that one.

[mmenke, 2014-05-29 19:54:16]

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
File net/websockets/websocket_job_test.cc (right):

https://codereview.chromium.org/295383007/diff/10011/net/websockets/websocket...
net/websockets/websocket_job_test.cc:277: MockHttpTransactionFactory(NextProto
next_proto, OrderedSocketData* data,
On 2014/05/29 19:26:50, Ryan Hamilton wrote:
> nit: 1 per line, since it no longer fits on one line.

Did this one, too, just forgot to response.

[Adam Rice, 2014-05-30 01:06:53]

lgtm

I should probably mention that WS/SPDY is unlikely to happen now, but WS/HTTP2
is looking reasonably likely. I assume we can just use this flag for that?

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
I assume the argument about the usefulness of Optional<bool> has long since
blown over, so I won't restart it. Unless you want to.

[mmenke, 2014-05-30 01:46:46]

I'm certainly fine with using it to mean WS over HTTP/2, though may mean it
should be renamed at some point.

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
On 2014/05/30 01:06:53, Adam Rice wrote:
> I assume the argument about the usefulness of Optional<bool> has long since
> blown over, so I won't restart it. Unless you want to.

I have no real opinion, just keeping the existing style.

Actually, my preference would be to get rid of all these we could, and just read
command line flags directly whenever initializing a context.  Reading the flags
and copying values seems weird (Except for globals), and putting them here
encourages use externally as well, and any code that manually copies these
values is bound to get out of date.

[Ryan Hamilton, 2014-05-30 02:37:02]

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
On 2014/05/30 01:46:46, mmenke wrote:
> On 2014/05/30 01:06:53, Adam Rice wrote:
> > I assume the argument about the usefulness of Optional<bool> has long since
> > blown over, so I won't restart it. Unless you want to.
> 
> I have no real opinion, just keeping the existing style.
> 
> Actually, my preference would be to get rid of all these we could, and just
read
> command line flags directly whenever initializing a context.  Reading the
flags
> and copying values seems weird (Except for globals), and putting them here
> encourages use externally as well, and any code that manually copies these
> values is bound to get out of date.

I'm not sure what the objection to Optional<> is here. The idea is that
HttpNetworkSession::Params gets to define the defaults for various options. As a
result, IOThread only sets the params that it needs to override. For example, if
neither --enable-quic nor --disable-quic are present (and none of the QUIC field
trials are active) then IOThread leaves the QUIC state alone. I'm happy to
consider an alternative approach than Optional<>, so long as it retains these
semantics.

[Adam Rice, 2014-05-30 03:25:23]

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
On 2014/05/30 02:37:02, Ryan Hamilton wrote:
> On 2014/05/30 01:46:46, mmenke wrote:
> > On 2014/05/30 01:06:53, Adam Rice wrote:
> > > I assume the argument about the usefulness of Optional<bool> has long
since
> > > blown over, so I won't restart it. Unless you want to.
> > 
> > I have no real opinion, just keeping the existing style.
> > 
> > Actually, my preference would be to get rid of all these we could, and just
> read
> > command line flags directly whenever initializing a context.  Reading the
> flags
> > and copying values seems weird (Except for globals), and putting them here
> > encourages use externally as well, and any code that manually copies these
> > values is bound to get out of date.
> 
> I'm not sure what the objection to Optional<> is here. The idea is that
> HttpNetworkSession::Params gets to define the defaults for various options. As
a
> result, IOThread only sets the params that it needs to override. For example,
if
> neither --enable-quic nor --disable-quic are present (and none of the QUIC
field
> trials are active) then IOThread leaves the QUIC state alone. I'm happy to
> consider an alternative approach than Optional<>, so long as it retains these
> semantics.

Well, in this case there's no "disable" flag, so we're essentially just using
Optional for a rainy day.

An alternative would be to define the defaults here instead of in
HttpNetworkSession::Params. If you wanted to be fancy you could do something
like

Defaulted<bool, false> enable_quic;

[Ryan Hamilton, 2014-05-30 03:43:26]

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/50001/chrome/browser/io_thread...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
On 2014/05/30 03:25:24, Adam Rice wrote:
> On 2014/05/30 02:37:02, Ryan Hamilton wrote:
> > On 2014/05/30 01:46:46, mmenke wrote:
> > > On 2014/05/30 01:06:53, Adam Rice wrote:
> > > > I assume the argument about the usefulness of Optional<bool> has long
> since
> > > > blown over, so I won't restart it. Unless you want to.
> > > 
> > > I have no real opinion, just keeping the existing style.
> > > 
> > > Actually, my preference would be to get rid of all these we could, and
just
> > read
> > > command line flags directly whenever initializing a context.  Reading the
> > flags
> > > and copying values seems weird (Except for globals), and putting them here
> > > encourages use externally as well, and any code that manually copies these
> > > values is bound to get out of date.
> > 
> > I'm not sure what the objection to Optional<> is here. The idea is that
> > HttpNetworkSession::Params gets to define the defaults for various options.
As
> a
> > result, IOThread only sets the params that it needs to override. For
example,
> if
> > neither --enable-quic nor --disable-quic are present (and none of the QUIC
> field
> > trials are active) then IOThread leaves the QUIC state alone. I'm happy to
> > consider an alternative approach than Optional<>, so long as it retains
these
> > semantics.
> 
> Well, in this case there's no "disable" flag, so we're essentially just using
> Optional for a rainy day.

*nod*

> An alternative would be to define the defaults here instead of in
> HttpNetworkSession::Params. If you wanted to be fancy you could do something
> like
> 
> Defaulted<bool, false> enable_quic;

We definitely do not want the defaults to be defined here :> The defaults should
live in net/ so that our unit tests can run with the right config by default.

[mmenke, 2014-05-30 17:23:25]

[+jhawkins]:  Mine doing an owners review of chrome/browser/io_thread.*?

[James Hawkins, 2014-05-30 17:30:07]

https://codereview.chromium.org/295383007/diff/160001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
Can you move this to the spdy block above?  Like maybe line 169 + \n.

[mmenke, 2014-05-30 17:45:30]

https://codereview.chromium.org/295383007/diff/160001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/295383007/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread.h:185: Optional<bool> enable_websocket_over_spdy;
On 2014/05/30 17:30:07, James Hawkins wrote:
> Can you move this to the spdy block above?  Like maybe line 169 + \n.

Done.  I was thinking of it as a new websocket block, but may make more sense
with the SPDY stuff.

[James Hawkins, 2014-06-01 16:22:24]

lgtm

[mmenke, 2014-06-02 17:08:52]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-06-02 17:09:40]

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

[mmenke, 2014-06-02 18:10:35]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-06-02 18:12:12]

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

[commit-bot: I haz the power, 2014-06-02 22:20:06]

Change committed as 274351