Add support for setting QUIC connection options via field trial config.

Add support for setting QUIC connection options via field trial config and command line.

Refactor IOThread's QUIC configuration support so that it can be unit
tested, and write unit tests.

Fixed a bug where params->forced_spdy_exclusions was not being set in InitializeNetworkSessionParams. Go go gadget const :>

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

[jar (doing other things), 2014-06-20 23:25:13]

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:986: params->forced_spdy_exclusions =
globals.forced_spdy_exclusions;
Nice bug fix.  +1 for the power of const?

This might be worth a comment in the bug, or almost a separate bug-fix CL....
but I don't know that anyone is really using this much.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1179: if
(quic_trial_group.ends_with(kQuicFieldTrialPacingSuffix))
I don't think you're consistent about whether "old" (name based) or "new"
(params based) selection has priority.

In this case, you give precedence to the name.

Line 1250, you give precedence to params.

Probably better would be consistency... and since we want params to be the
future... I'd suggest letting them take the lead (be checked first).

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1199: for (std::vector<std::string>::iterator token
= tokens.begin();
nit: Add comment that tokens are expected to be no more than 4 characters in
length... but we handle oversize gracefully.

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:295: base::StringPiece quic_trial_group,
nit: should this be a const ref?

I think it is unusual in Chrome to not use const (on string pieces), but I just
noticed (looking at other code in net) that it is sometimes used without the
const.

I *think* the right thing is to use the const + ref in arg passing (as is
regularly done in string_piece.h).

same thing later in file.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:338: const std::map<std::string, std::string>&
quic_trial_params);
nit: I think it would be helpful to decide a typedef for this map.  Maybe:

FinchParameters

It is used numerous times in this file, and made reading harder (to have to
re-parse this each time).

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:154:
EXPECT_TRUE(params.enable_quic_time_based_loss_detection);
nit: FWIW: I don't think you ever test to see if this value is disabled.  Maybe
an initial "default" test that checks all the initial values are "as expected"
when you have no forcing.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:176: EXPECT_EQ(1350u,
params.quic_max_packet_length);
nit: nicer would be to use different values, so be sure we parse the real value,
or at least check that sometimes we get a value other than 1350, such as the
default(?) value, which maybe you could snarf in the test startup, or such.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:213:
supported_versions.push_back(net::QUIC_VERSION_18);
Will this test fail as we stop supporting version 18?

[Ryan Hamilton, 2014-06-21 02:45:23]

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:986: params->forced_spdy_exclusions =
globals.forced_spdy_exclusions;
On 2014/06/20 23:25:12, jar wrote:
> Nice bug fix.  +1 for the power of const?

Yeah, const rocks :>

> This might be worth a comment in the bug, or almost a separate bug-fix CL....
> but I don't know that anyone is really using this much.

I'd rather not delay this CL while I wait for the bug-fix CL to land :< Is that
ok?

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1179: if
(quic_trial_group.ends_with(kQuicFieldTrialPacingSuffix))
On 2014/06/20 23:25:12, jar wrote:
> I don't think you're consistent about whether "old" (name based) or "new"
> (params based) selection has priority.
> 
> In this case, you give precedence to the name.
> 
> Line 1250, you give precedence to params.
> 
> Probably better would be consistency... and since we want params to be the
> future... I'd suggest letting them take the lead (be checked first).

Good point. Done.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1199: for (std::vector<std::string>::iterator token
= tokens.begin();
On 2014/06/20 23:25:12, jar wrote:
> nit: Add comment that tokens are expected to be no more than 4 characters in
> length... but we handle oversize gracefully.

Done.

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:295: base::StringPiece quic_trial_group,
On 2014/06/20 23:25:12, jar wrote:
> nit: should this be a const ref?
> 
> I think it is unusual in Chrome to not use const (on string pieces), but I
just
> noticed (looking at other code in net) that it is sometimes used without the
> const.
> 
> I *think* the right thing is to use the const + ref in arg passing (as is
> regularly done in string_piece.h).
> 
> same thing later in file.

As I understand it, it's actually faster to pass in a StringPiece by value then
it is to pass a ref. We consistently pass by value in QUIC code, fwiw.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.h:338: const std::map<std::string, std::string>&
quic_trial_params);
On 2014/06/20 23:25:12, jar wrote:
> nit: I think it would be helpful to decide a typedef for this map.  Maybe:
> 
> FinchParameters
> 
> It is used numerous times in this file, and made reading harder (to have to
> re-parse this each time).

Done. (Yeah, I wish there were such a typedef in the finch .h file.)

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:154:
EXPECT_TRUE(params.enable_quic_time_based_loss_detection);
On 2014/06/20 23:25:13, jar wrote:
> nit: FWIW: I don't think you ever test to see if this value is disabled. 
Maybe
> an initial "default" test that checks all the initial values are "as expected"
> when you have no forcing.

Done.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:176: EXPECT_EQ(1350u,
params.quic_max_packet_length);
On 2014/06/20 23:25:13, jar wrote:
> nit: nicer would be to use different values, so be sure we parse the real
value,
> or at least check that sometimes we get a value other than 1350, such as the
> default(?) value, which maybe you could snarf in the test startup, or such.

I'm now checking that it's set to 1200 by default in the first Enabled test.
Does that alleviate your concerns about 1350?

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:213:
supported_versions.push_back(net::QUIC_VERSION_18);
On 2014/06/20 23:25:12, jar wrote:
> Will this test fail as we stop supporting version 18?

Indeed. Refactored to programmatically use the oldest supported version.

[jar (doing other things), 2014-06-22 18:48:34]

LGTM % nits

Please add "CL level comment" about the bug you saw/fixed as well (see mention
below).

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:986: params->forced_spdy_exclusions =
globals.forced_spdy_exclusions;
On 2014/06/21 02:45:23, Ryan Hamilton wrote:
> On 2014/06/20 23:25:12, jar wrote:
> > Nice bug fix.  +1 for the power of const?
> 
> Yeah, const rocks :>
> 
> > This might be worth a comment in the bug, or almost a separate bug-fix
CL....
> > but I don't know that anyone is really using this much.
> 
> I'd rather not delay this CL while I wait for the bug-fix CL to land :< Is
that
> ok?

Comment in this CL (top level) is fine.
thanks!

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1274: if
(!command_line.HasSwitch(switches::kQuicVersion)) {
This looks questionable (probably shouldn't have been at this mid-patch)... but
the latest refactor got rid of it.  If I misunderstand... please comment.

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:176: EXPECT_EQ(1350u,
params.quic_max_packet_length);
On 2014/06/21 02:45:23, Ryan Hamilton wrote:
> On 2014/06/20 23:25:13, jar wrote:
> > nit: nicer would be to use different values, so be sure we parse the real
> value,
> > or at least check that sometimes we get a value other than 1350, such as the
> > default(?) value, which maybe you could snarf in the test startup, or such.
> 
> I'm now checking that it's set to 1200 by default in the first Enabled test.
> Does that alleviate your concerns about 1350? 

Perfect.  Thanks.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:213:
supported_versions.push_back(net::QUIC_VERSION_18);
On 2014/06/21 02:45:23, Ryan Hamilton wrote:
> On 2014/06/20 23:25:12, jar wrote:
> > Will this test fail as we stop supporting version 18?
> 
> Indeed. Refactored to programmatically use the oldest supported version.

Perfect. Thanks.

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

https://codereview.chromium.org/346043002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1284: net::QuicVersionVector supported_versions =
net::QuicSupportedVersions();
nit: With latest refactor, this is unused in this method.

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

https://codereview.chromium.org/346043002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:352: static net::QuicTagVector
ParseQuicConnectionOptions(
nit: Add comment description

[Ryan Hamilton, 2014-06-22 19:48:57]

jochen: chrome/ OWNERS review please.

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

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:986: params->forced_spdy_exclusions =
globals.forced_spdy_exclusions;
On 2014/06/22 18:48:34, jar wrote:
> On 2014/06/21 02:45:23, Ryan Hamilton wrote:
> > On 2014/06/20 23:25:12, jar wrote:
> > > Nice bug fix.  +1 for the power of const?
> > 
> > Yeah, const rocks :>
> > 
> > > This might be worth a comment in the bug, or almost a separate bug-fix
> CL....
> > > but I don't know that anyone is really using this much.
> > 
> > I'd rather not delay this CL while I wait for the bug-fix CL to land :< Is
> that
> > ok?
> 
> Comment in this CL (top level) is fine.
> thanks!
> 

Done. Good idea.

https://codereview.chromium.org/346043002/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:1274: if
(!command_line.HasSwitch(switches::kQuicVersion)) {
On 2014/06/22 18:48:34, jar wrote:
> This looks questionable (probably shouldn't have been at this mid-patch)...
but
> the latest refactor got rid of it.  If I misunderstand... please comment.

Yeah, that was ... wrong :> As you surmised, it's fixed now.

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

https://codereview.chromium.org/346043002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1284: net::QuicVersionVector supported_versions =
net::QuicSupportedVersions();
On 2014/06/22 18:48:34, jar wrote:
> nit: With latest refactor, this is unused in this method.

Doh! Thanks.

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

https://codereview.chromium.org/346043002/diff/120001/chrome/browser/io_threa...
chrome/browser/io_thread.h:352: static net::QuicTagVector
ParseQuicConnectionOptions(
On 2014/06/22 18:48:34, jar wrote:
> nit: Add comment description

Done.

[Ryan Hamilton, 2014-06-23 15:42:44]

This time really adding jochen.

jochen: chrome/ OWNERS review, please.

[Ryan Hamilton, 2014-06-23 21:06:59]

thestig: chrome/ OWNERS review.

[Lei Zhang, 2014-06-23 21:28:14]

lgtm with some nits

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:294: std::string key) {
const ref

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:297: return "";
I think GetVariationParam() can return a const ref, in which case, you can
substitute in base::EmptyString() here.

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1191: return ParseQuicConnectionOptions(
nit: put this block in braces.

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:230: typedef std::map<std::string, std::string>
VariationParameters;
Can you briefly document what the key/value are?

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:318: base::StringPiece quic_trial_group,
base::StringPiece should be const ref, ditto elsewhere

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
it's 2014

https://codereview.chromium.org/346043002/diff/140001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/346043002/diff/140001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1815: 'browser/io_thread_unittest.cc',
nit: can you put this in alphabetical order? (and the two below if you don't
mind)

[Ryan Hamilton, 2014-06-23 22:38:52]

Thanks for the quick response!

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:294: std::string key) {
On 2014/06/23 21:28:14, Lei Zhang wrote:
> const ref

Done. Good point

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:297: return "";
On 2014/06/23 21:28:14, Lei Zhang wrote:
> I think GetVariationParam() can return a const ref, in which case, you can
> substitute in base::EmptyString() here.

Done.

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1191: return ParseQuicConnectionOptions(
On 2014/06/23 21:28:14, Lei Zhang wrote:
> nit: put this block in braces.

Done.

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:230: typedef std::map<std::string, std::string>
VariationParameters;
On 2014/06/23 21:28:14, Lei Zhang wrote:
> Can you briefly document what the key/value are?

Done.

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:318: base::StringPiece quic_trial_group,
On 2014/06/23 21:28:14, Lei Zhang wrote:
> base::StringPiece should be const ref, ditto elsewhere

Hm. The internal copy of string_piece.h says:

// Prefer passing StringPieces by value:

And it links to a long discussion about why that is ever so slightly faster, and
produces smaller code. If you feel strongly, I can change this. It makes me
wonder if we should sync the advice between the internal and external versions
of string_piece.h

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread_unittest.cc:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
On 2014/06/23 21:28:14, Lei Zhang wrote:
> it's 2014

Done. Indeed it is!

https://codereview.chromium.org/346043002/diff/140001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/346043002/diff/140001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1815: 'browser/io_thread_unittest.cc',
On 2014/06/23 21:28:14, Lei Zhang wrote:
> nit: can you put this in alphabetical order? (and the two below if you don't
> mind)

Done.

[Lei Zhang, 2014-06-23 22:42:06]

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

https://codereview.chromium.org/346043002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:318: base::StringPiece quic_trial_group,
On 2014/06/23 22:38:52, Ryan Hamilton wrote:
> On 2014/06/23 21:28:14, Lei Zhang wrote:
> > base::StringPiece should be const ref, ditto elsewhere
> 
> Hm. The internal copy of string_piece.h says:
> 
> // Prefer passing StringPieces by value:
> 
> And it links to a long discussion about why that is ever so slightly faster,
and
> produces smaller code. If you feel strongly, I can change this. It makes me
> wonder if we should sync the advice between the internal and external versions
> of string_piece.h

Ok, I should have read string_piece.h, but instead I just looked at a few sample
files on cs and they were all by const ref.

[Ryan Hamilton, 2014-06-24 03:04:30]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-06-24 03:05:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/346043002/180001

[commit-bot: I haz the power, 2014-06-24 06:00:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 06:21:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 06:21:32]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Ryan Hamilton, 2014-06-24 15:35:30]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-06-24 15:36:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/346043002/200001

[commit-bot: I haz the power, 2014-06-24 18:50:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Ryan Hamilton, 2014-06-24 18:54:06]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-06-24 18:55:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/346043002/220001

[Ryan Hamilton, 2014-06-25 03:28:57]

The CQ bit was unchecked by rch@chromium.org

[Ryan Hamilton, 2014-06-25 03:29:09]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-06-25 03:31:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/346043002/220001

[commit-bot: I haz the power, 2014-06-25 04:37:52]

Change committed as 279584