QUIC - enabled FLAGS_allow_truncated_connection_ids_for_quic

QUIC - enabled FLAGS_allow_truncated_connection_ids_for_quic.

Set up a FieldTrial which could disable this flag (via finch).

When the flag is enabled, chrome sends TCID parameter in CHLO message.
It is set to 0.

Added ??ByteConnectionId unittests to end_to_end_test.cc (found while
comparing the chromium code with internal source tree).

R=rch@chromium.org

Committed: https://crrev.com/6a4cf3b4810d8391b0962dbacd094b4e42edd6c0
Cr-Commit-Position: refs/heads/master@{#310705}

[Ryan Hamilton, 2015-01-07 23:10:23]

https://codereview.chromium.org/804813010/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/804813010/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:92: // by default.
I think it would be better to make this finch controlled so that we can turn it
off if there are any problems. What do you think?

I thought we also decided to set the FLAG to true by default?

[ramant (doing other things), 2015-01-08 02:54:51]

Hi Ryan,
  This is work-in-progress. Was able to verify that all unittests pass. Would
like to talk to you if there is a better approach.

thanks
raman

https://codereview.chromium.org/804813010/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/804813010/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:92: // by default.
On 2015/01/07 23:10:23, Ryan Hamilton wrote:
> I think it would be better to make this finch controlled so that we can turn
it
> off if there are any problems. What do you think?
> 
> I thought we also decided to set the FLAG to true by default?

Done.

[Ryan Hamilton, 2015-01-08 18:35:44]

https://codereview.chromium.org/804813010/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/804813010/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:341: 1, kClientDataStreamId1,
PACKET_0BYTE_CONNECTION_ID, true, true,
As discussed offline, we should probably change the MockCryptoClientStream to
specify a TCID of 8.

[ramant (doing other things), 2015-01-08 22:16:17]

Patchset #3 (id:40001) has been deleted

[ramant (doing other things), 2015-01-08 22:21:15]

Thanks very much for the offline suggestions. Made all the changes we had talked
about. All QUIC unit tests pass (reverted all the changes in Patch Set 2). PTAL.

https://codereview.chromium.org/804813010/diff/20001/net/quic/quic_network_tr...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/804813010/diff/20001/net/quic/quic_network_tr...
net/quic/quic_network_transaction_unittest.cc:341: 1, kClientDataStreamId1,
PACKET_0BYTE_CONNECTION_ID, true, true,
On 2015/01/08 18:35:44, Ryan Hamilton wrote:
> As discussed offline, we should probably change the MockCryptoClientStream to
> specify a TCID of 8.

Done.

[Ryan Hamilton, 2015-01-08 22:28:07]

Almost there.

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

https://codereview.chromium.org/804813010/diff/60001/chrome/browser/io_thread...
chrome/browser/io_thread.h:197: Optional<bool>
quic_disable_truncated_connection_ids;
I think we usually use enable_foo for new features. Can you reverse the polarity
to be quic_enable_truncated_connection_ids?

This also means that no field trials groups will use the new code until we
explicitly add a new field trial param which I think is a good idea, but I could
be wrong...?

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:600:
FLAGS_allow_truncated_connection_ids_for_quic = false;
Instead of assigning to FLAGS_ here, how 'bout checking
disable_truncated_connection_ids_ directly down on line 1033?

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory_test.cc:168:
/*disable_truncated_connection_ids=*/false,
Can we set this to true (to make sure things work with the new code)

[ramant (doing other things), 2015-01-08 23:06:58]

Hi Ryan,
  I hope I got the default values for the flag right. After these changes get
submitted, will update the finch trial groups to explicitly enable the new
param. PTAL.

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

https://codereview.chromium.org/804813010/diff/60001/chrome/browser/io_thread...
chrome/browser/io_thread.h:197: Optional<bool>
quic_disable_truncated_connection_ids;
On 2015/01/08 22:28:07, Ryan Hamilton wrote:
> I think we usually use enable_foo for new features. Can you reverse the
polarity
> to be quic_enable_truncated_connection_ids?
> 
> This also means that no field trials groups will use the new code until we
> explicitly add a new field trial param which I think is a good idea, but I
could
> be wrong...?

Done.

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:600:
FLAGS_allow_truncated_connection_ids_for_quic = false;
On 2015/01/08 22:28:07, Ryan Hamilton wrote:
> Instead of assigning to FLAGS_ here, how 'bout checking
> disable_truncated_connection_ids_ directly down on line 1033? 

Done.

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/804813010/diff/60001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory_test.cc:168:
/*disable_truncated_connection_ids=*/false,
On 2015/01/08 22:28:07, Ryan Hamilton wrote:
> Can we set this to true (to make sure things work with the new code)

Hope I got this change right.

Done.

[Ryan Hamilton, 2015-01-08 23:17:00]

LGTM. Tow tiny changes.

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

https://codereview.chromium.org/804813010/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:149:
EXPECT_FALSE(params.quic_enable_truncated_connection_ids);
No need to test for this in all the tests. Just the previous on and the test
specific to this flag.

https://codereview.chromium.org/804813010/diff/80001/net/http/http_network_se...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/804813010/diff/80001/net/http/http_network_se...
net/http/http_network_session.cc:94: quic_enable_truncated_connection_ids(true),
I think we want this to default to false.

[ramant (doing other things), 2015-01-08 23:40:21]

Thanks very much Ryan for all the comments and offline debugging help,
raman

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

https://codereview.chromium.org/804813010/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread_unittest.cc:149:
EXPECT_FALSE(params.quic_enable_truncated_connection_ids);
On 2015/01/08 23:17:00, Ryan Hamilton wrote:
> No need to test for this in all the tests. Just the previous on and the test
> specific to this flag.

Done.

https://codereview.chromium.org/804813010/diff/80001/net/http/http_network_se...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/804813010/diff/80001/net/http/http_network_se...
net/http/http_network_session.cc:94: quic_enable_truncated_connection_ids(true),
On 2015/01/08 23:17:00, Ryan Hamilton wrote:
> I think we want this to default to false.

Done.

[ramant (doing other things), 2015-01-08 23:40:33]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:41:38]

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

[commit-bot: I haz the power, 2015-01-09 01:38:04]

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

[commit-bot: I haz the power, 2015-01-09 01:38:05]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[ramant (doing other things), 2015-01-09 02:43:51]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-01-09 02:45:16]

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

[commit-bot: I haz the power, 2015-01-09 03:41:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-01-09 03:42:22]

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