Plumbing for TCP FastOpen field trial and enables it for SSL sockets.

Plumbing for TCP FastOpen field trial.

BUG=413370, 402005
Committed: https://crrev.com/f9b4bec613a221bf74be814775a8b2b035604939
Cr-Commit-Position: refs/heads/master@{#294829}

[Jana, 2014-09-10 16:35:59]

jri@chromium.org changed reviewers:
+ mmenke@chromium.org, rdsmith@chromium.org

[Jana, 2014-09-10 16:36:00]

This CL does the requisite plumbing to enable TCP FastOpen when in a field trial
and enables it for SSL sockets when in the "TCPFastOpenEnabledForSSL" field
trial.

[mmenke, 2014-09-10 18:29:09]

LGTM

https://codereview.chromium.org/557373002/diff/20001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/557373002/diff/20001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:239:
TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT;
Optional:  You could move combine_connect_and_write to the top of this function,
and have:

TransportSocketParams::CombineConnectAndWritePolicy combine_connect_and_write =
TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT;
if (using_ssl || proxy_info.is_https())
  combine_connect_and_write =
TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DESIRED;

And use it for transport socket params everywhere.  That'd catch the SSL over
non-proxy SSL case, though I suppose connecting to a non-SSL proxy could
theoretically be non-idempotent...Anyways, fine as-is.

[Randy Smith (Not in Mondays), 2014-09-10 18:57:06]

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

https://codereview.chromium.org/557373002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:856:
globals_->enable_tcp_fast_open_for_ssl.set(true);
This looks wrong to me--it looks like you're conflating the field trial name
with the group within the trial that you're going to use.  Usually field trials
have 3 groups: Enabled/Disabled/Default (often named just that) and you select
among them.  You can take a look at how I'm doing SDCH in chrome_browser_main.cc
(search for "SDCH" to see what I mean.

More generally, I'd recommend you do the finch bug to setup a field trial, get a
finch ambassador assigned, and ask them to review this CL.  They often have
preferences about how you do things, and catching them earlier rather than later
is helpful.

[Jana, 2014-09-11 20:04:59]

Thanks, Matt and Randy. I've fixed the code, PTAL. I've also created a bug per
Randy's suggestion (https://code.google.com/p/chromium/issues/detail?id=413370).
Should I wait for an ambassador to now be assigned?

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

https://codereview.chromium.org/557373002/diff/20001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:856:
globals_->enable_tcp_fast_open_for_ssl.set(true);
On 2014/09/10 18:57:06, rdsmith wrote:
> This looks wrong to me--it looks like you're conflating the field trial name
> with the group within the trial that you're going to use.  Usually field
trials
> have 3 groups: Enabled/Disabled/Default (often named just that) and you select
> among them.  You can take a look at how I'm doing SDCH in
chrome_browser_main.cc
> (search for "SDCH" to see what I mean.
> 
> More generally, I'd recommend you do the finch bug to setup a field trial, get
a
> finch ambassador assigned, and ask them to review this CL.  They often have
> preferences about how you do things, and catching them earlier rather than
later
> is helpful.

Yes, you are right -- I had conflated these things. I've separated them out, now
that I (yet again) understand Finch better. PTAL.

https://codereview.chromium.org/557373002/diff/20001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/557373002/diff/20001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:239:
TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT;
On 2014/09/10 18:29:08, mmenke wrote:
> Optional:  You could move combine_connect_and_write to the top of this
function,
> and have:
> 
> TransportSocketParams::CombineConnectAndWritePolicy combine_connect_and_write
=
> TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT;
> if (using_ssl || proxy_info.is_https())
>   combine_connect_and_write =
> TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DESIRED;
> 
> And use it for transport socket params everywhere.  That'd catch the SSL over
> non-proxy SSL case, though I suppose connecting to a non-SSL proxy could
> theoretically be non-idempotent...Anyways, fine as-is.

Since it's not a strong preference for you, I'll leave it as is. The decision is
local to the TransportSocketParams below, and it makes the code easier to read
and reason about, IMO.

[Jana, 2014-09-12 18:19:07]

jri@chromium.org changed reviewers:
+ hodie@chromium.org

[Jana, 2014-09-12 18:19:08]

+hodie for a review on use of the Field Trial bits.

[Jana, 2014-09-12 18:24:05]

jri@chromium.org changed reviewers:
+ asvitkine@chromium.org
- hodie@chromium.org

[Jana, 2014-09-12 18:24:06]

-hodie, +asvitkine for review of the Finch bits.

[Alexei Svitkine (slow), 2014-09-12 18:27:56]

field trial code lgtm, but please associate this CL with a crbug

[Jana, 2014-09-12 18:33:56]

Added bug id. Thanks, Alexei!

[Jana, 2014-09-13 00:11:33]

Thanks all, I'm going to submit this patch. Randy, I'm assuming that you're fine
with the rest of the patch (the non-finch parts) -- I'm happy to make any mods
next week if you have any more feedback.

[Jana, 2014-09-13 00:11:41]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:18:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557373002/40001

[commit-bot: I haz the power, 2014-09-13 06:19:31]

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

[commit-bot: I haz the power, 2014-09-13 06:19:32]

Exceeded time limit waiting for builds to trigger.

[Jana, 2014-09-15 15:45:39]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2014-09-15 15:46:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557373002/40001

[commit-bot: I haz the power, 2014-09-15 15:48:03]

Committed patchset #3 (id:40001) as 3fec0a3ef77c15c3e694d3f347d40d73b93f7531

[commit-bot: I haz the power, 2014-09-15 15:50:56]

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

[Randy Smith (Not in Mondays), 2014-09-15 20:10:18]

On 2014/09/13 00:11:33, Jana wrote:
> Thanks all, I'm going to submit this patch. Randy, I'm assuming that you're
fine
> with the rest of the patch (the non-finch parts) -- I'm happy to make any mods
> next week if you have any more feedback.

Yep, I'm good.  When I scanned the code, I thought there was still a bug in the
finch trial code (overriding of finch trial via command line flag should result
in removal from the finch trial so we don't skew statistics) but Alexei
understands that stuff far far better than I, so I defer to him.  LGTM.

[Jana, 2014-09-15 21:15:49]

Thanks, Randy! The way QUIC disables experiments on a user flag is by using
"forcing_flag" in the experiments json file... asvitkine@ can correct me if this
is not the method to be used. Otherwise, that's what I'm planning to do here as
well.

[Alexei Svitkine (slow), 2014-09-15 21:54:22]

Yeah, the way the Finch mechanism works is you can associate a group with a
flag, so if that flag is seen on startup, the specific Finch group is selected.