Independently enable SPDY versions from field trial.

Independently enable SPDY versions from field trial.

Add field trial parameters for independently enabling SPDY/3.1, HTTP/2 draft-14,
and HTTP/2 protocols.  Add a field trial group prefix to trigger parsing
parameters, so that behavior of current field trial groups is preserved.
Refactor SPDY parameter parsing code to make it similar to QUIC code, also in
tests.  Write tests for new functionality, for default behavior, and for command
line parsing.

BUG=469181
Committed: https://crrev.com/bc0b05bd43245357433854cba3179f28ad52d5f0
Cr-Commit-Position: refs/heads/master@{#322216}

[Bence, 2015-03-20 21:33:44]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2015-03-20 21:33:45]

Ryan, PTAL.  Thanks.

[Ryan Hamilton, 2015-03-20 23:38:08]

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:931: if (command_line.HasSwitch(switches::kUseSpdy))
{
Let's use early-return to clean up this mess :>

if (command_line.HasSwitch(switches::kUseSpdy)) {
  ...
  return;
}

if (command_line.HasSwitch(switches::kEnableSpdy4)) {
  ...
  return;
}

if (command_line.HasSwitch(switches::kEnableNpnHttpOnly)) { 
  ...
  return;
}

if (spdy_trial_group.starts_with(kSpdyFieldTrialHoldbackGroupNamePrefix)) {
  ...
  return;
}

if (spdy_trial_group.starts_with(kSpdyFieldTrialSpdy31GroupNamePrefix)) {
  ...
  return;
}
  
if (spdy_trial_group.starts_with(kSpdyFieldTrialSpdy4GroupNamePrefix)) {
  ...
  return;
}

if (spdy_trial_group.starts_with(kSpdyFieldTrialParametrizedPrefix)) {
  ...
  return;
}

// By default, enable HTTP/2.
globals->next_protos = net::NextProtos(true, true, true, true);
globals->use_alternate_protocols.set(true);
}

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:934: EnableSpdy(spdy_mode, globals);
Ah, so here's the one place this method is called. Perhaps it should be called
something like ConfigureSpdyGlobalsFromUseSpdyArgument() or some such.

Since it's a helper of ConfigureSpdy, you could also make it a method in an
anon-namespace of this class. Your call.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:957: GetVariationParam(spdy_trial_params,
"quic_enabled"), "true");
I think this is going to fight with the QUIC field trial which tries to be
responsible for enabling QUIC.

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

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.h:289: static void EnableSpdy(const std::string& mode,
IOThread::Globals* globals);
Hm. This method doesn't actually "enable spdy", rather it sets up the spdy
globals, right? Sounds like the same thing the method below does. I think we
need a better name.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:151:
command_line_.AppendSwitch("enable-spdy4");
As long as you're testing the command line handling (thank you!!!!!!!) can you
also test --use-spdy?

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.cc
File net/socket/next_proto.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.c...
net/socket/next_proto.cc:31: return NextProtos(true, true, false, false);
Lots of booleans is a recipe for typos. How about just taking a NextProtoVector
directly? (Perhaps performing validation on it that it only contains the protos
you would like the caller to be able to use?

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h...
net/socket/next_proto.h:60: NET_EXPORT NextProtoVector
NextProtosWithSpdyAndQuic(bool spdy_enabled,
nit: newline before

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h...
net/socket/next_proto.h:61: bool quic_enabled);
Do we really need this method since the previous one basically give us all we
could ask for?

[Bence, 2015-03-23 14:19:14]

Ryan, PTAL.  Thanks.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:931: if (command_line.HasSwitch(switches::kUseSpdy))
{
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> Let's use early-return to clean up this mess :>
> 
> if (command_line.HasSwitch(switches::kUseSpdy)) {
>   ...
>   return;
> }
> 
> if (command_line.HasSwitch(switches::kEnableSpdy4)) {
>   ...
>   return;
> }
> 
> if (command_line.HasSwitch(switches::kEnableNpnHttpOnly)) { 
>   ...
>   return;
> }
> 
> if (spdy_trial_group.starts_with(kSpdyFieldTrialHoldbackGroupNamePrefix)) {
>   ...
>   return;
> }
> 
> if (spdy_trial_group.starts_with(kSpdyFieldTrialSpdy31GroupNamePrefix)) {
>   ...
>   return;
> }
>   
> if (spdy_trial_group.starts_with(kSpdyFieldTrialSpdy4GroupNamePrefix)) {
>   ...
>   return;
> }
> 
> if (spdy_trial_group.starts_with(kSpdyFieldTrialParametrizedPrefix)) {
>   ...
>   return;
> }
> 
> // By default, enable HTTP/2.
> globals->next_protos = net::NextProtos(true, true, true, true);
> globals->use_alternate_protocols.set(true);
> }

Done.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:934: EnableSpdy(spdy_mode, globals);
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> Ah, so here's the one place this method is called. Perhaps it should be called
> something like ConfigureSpdyGlobalsFromUseSpdyArgument() or some such.
> 
> Since it's a helper of ConfigureSpdy, you could also make it a method in an
> anon-namespace of this class. Your call.

Great idea.  Done.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:957: GetVariationParam(spdy_trial_params,
"quic_enabled"), "true");
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> I think this is going to fight with the QUIC field trial which tries to be
> responsible for enabling QUIC.

Good point.  I have the impression that this is preexisting problem.  I'm
solving it by adding QUIC to next_protos iff enable_quic as determined earlier
by ConfigureQuic.

Done.

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

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.h:289: static void EnableSpdy(const std::string& mode,
IOThread::Globals* globals);
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> Hm. This method doesn't actually "enable spdy", rather it sets up the spdy
> globals, right? Sounds like the same thing the method below does. I think we
> need a better name.

Done.

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:151:
command_line_.AppendSwitch("enable-spdy4");
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> As long as you're testing the command line handling (thank you!!!!!!!) can you
> also test --use-spdy?

Done.

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.cc
File net/socket/next_proto.cc (right):

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.c...
net/socket/next_proto.cc:31: return NextProtos(true, true, false, false);
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> Lots of booleans is a recipe for typos. How about just taking a
NextProtoVector
> directly? (Perhaps performing validation on it that it only contains the
protos
> you would like the caller to be able to use?

Okay, you convinced me.  I'm moving the logic to io_thread.cc.

Done.

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h
File net/socket/next_proto.h (right):

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h...
net/socket/next_proto.h:60: NET_EXPORT NextProtoVector
NextProtosWithSpdyAndQuic(bool spdy_enabled,
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> nit: newline before

Moot.

https://codereview.chromium.org/1020363003/diff/20001/net/socket/next_proto.h...
net/socket/next_proto.h:61: bool quic_enabled);
On 2015/03/20 23:38:07, Ryan Hamilton wrote:
> Do we really need this method since the previous one basically give us all we
> could ask for?

Moot.

[Ryan Hamilton, 2015-03-23 21:29:14]

Great progress!

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:356: it != spdy_options.end(); ++it) {
nit: for (const std::string& element : spdy_options) {

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:406: continue;
I'd love to see these two options killed. :>

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:413: continue;
perhaps this belongs inside the {} so that if the value is invalid, we'll
LOG(DFATAL)? at line 418?

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:417: }
I can't quite work out if this is required. Can you say more?

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1083: if
(LowerCaseEqualsASCII(GetVariationParam(spdy_trial_params, "h2_enabled"),
I recommend you make all 3 use the same format:

* enable_spdy3_1
* enable_http2_14
* enable_http2

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1087: globals->use_alternate_protocols.set(true);
Does this mean there's no way to use the Parameteriezd group name to disable
SPDY? Might we ever want to do that?

[Bence, 2015-03-24 14:05:04]

Ryan, PTAL.

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:356: it != spdy_options.end(); ++it) {
On 2015/03/23 21:29:14, Ryan Hamilton wrote:
> nit: for (const std::string& element : spdy_options) { 

Done.

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:413: continue;
On 2015/03/23 21:29:13, Ryan Hamilton wrote:
> perhaps this belongs inside the {} so that if the value is invalid, we'll
> LOG(DFATAL)? at line 418?

Done.

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:417: }
On 2015/03/23 21:29:13, Ryan Hamilton wrote:
> I can't quite work out if this is required. Can you say more?

I have no idea either, so I'm removing it.

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1083: if
(LowerCaseEqualsASCII(GetVariationParam(spdy_trial_params, "h2_enabled"),
On 2015/03/23 21:29:13, Ryan Hamilton wrote:
> I recommend you make all 3 use the same format:
> 
> * enable_spdy3_1
> * enable_http2_14
> * enable_http2

Sorry, that was an oversite on my part.

Done.

https://codereview.chromium.org/1020363003/diff/40001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1087: globals->use_alternate_protocols.set(true);
On 2015/03/23 21:29:14, Ryan Hamilton wrote:
> Does this mean there's no way to use the Parameteriezd group name to disable
> SPDY? Might we ever want to do that?

Might we?  The primary goal for this functionality are disabling for deprecation
or because of interop issues.  Maybe we need something more than removing
SPDY/3.1 from ALPN advertised protocols?

[Ryan Hamilton, 2015-03-24 19:28:27]

lgtm

[Bence, 2015-03-25 11:37:16]

On 2015/03/24 19:28:27, Ryan Hamilton wrote:
> lgtm

Ryan, please take a look at patch set 5.  As you pointed out off-line,
HttpStreamFactory::set_spdy_enabled should also be toggled.  (If defaults to
true, it should be cleared if no SPDY variants are enabled.)  I also added a
TODO to get rid of it since this is some very obscure redundancy here between
HttpStreamFactory::spdy_enabled_ and globals->next_protos.  Thanks.

[Bence, 2015-03-25 14:15:51]

Patchset #6 (id:100001) has been deleted

[Ryan Hamilton, 2015-03-25 18:24:39]

lgtm

[Bence, 2015-03-25 18:35:27]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-25 18:36:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1020363003/80001

[commit-bot: I haz the power, 2015-03-25 19:51:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 19:52:34]

Patchset 5 (id:??) landed as
https://crrev.com/bc0b05bd43245357433854cba3179f28ad52d5f0
Cr-Commit-Position: refs/heads/master@{#322216}