Added a field trial (finch experiment) to disable SPDY for x% of users.

Added a field trial (finch experiment) to disable SPDY for x% of users.

BUG=297361, 299956
R=rch@chromium.org, asvitkine@chromium.org

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

[ramant (doing other things), 2013-09-27 23:16:39]

Small change to disable SPDY via finch experiment.

[Ryan Hamilton, 2013-09-28 13:58:39]

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

https://codereview.chromium.org/25087007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:733:
base::FieldTrialList::FindFullName(kSpdyFieldTrialName);
You need to make sure that you call FindFullName for every browser session, even
if it's disabled via command line flag.  You'll also need to add forcing flags
to the finch config.

You might want to ask the finch folks about what to do if spdy is disabled by
policy to make sure we account for that correctly.

[ramant (doing other things), 2013-09-29 23:41:32]

motek@: Called FindFullName for every session (independent of policy and command
line flags). Could you please take a look at this CL. Will send finch config
changes in a separate internal CL. thanks.

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

https://codereview.chromium.org/25087007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:733:
base::FieldTrialList::FindFullName(kSpdyFieldTrialName);
On 2013/09/28 13:58:40, Ryan Hamilton wrote:
> You need to make sure that you call FindFullName for every browser session,
even
> if it's disabled via command line flag.

Done.

>  You'll also need to add forcing flags
> to the finch config.

Will do.

> 
> You might want to ask the finch folks about what to do if spdy is disabled by
> policy to make sure we account for that correctly.

CC'ed marcin finch advisor (will find from him about what to do if SPDY is
disabled via policy)

[ramant (doing other things), 2013-09-30 18:11:26]

asvitkine@ would appreciate your comments and I got when we call FindFullName
when SPDY is disabled via policy and its impact on Finch experiment (and if it
pollutes the metrics or not).

[Alexei Svitkine (slow), 2013-09-30 19:40:13]

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

https://codereview.chromium.org/25087007/diff/11001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:720: if (command_line.HasSwitch(switches::kUseSpdy))
{
Either add all of these flags to the config for the study, or call
FindFullName() only after you've checked all the flags that are not in the
config.

(Calling it marks the trial as "used" and therefore will be reported in
metrics.)

Alternatively, for flags that are not in the config, you can call Disable() on
the FieldTrial object returned by Find() to also disable reporting for that
trial.

[motek., 2013-09-30 20:44:29]

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

https://codereview.chromium.org/25087007/diff/11001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:720: if (command_line.HasSwitch(switches::kUseSpdy))
{
From the Department of Redundancy Department: pretty much what asvitkine@ wrote.


Do all these flags affect your experiment in any material way? If not, it might
be best to split the chain of ifs into a part affected by the study and the
rest. 

You may also want to single out the case where SPDY is disabled by policy and
FieldTrialList::Find()->Disable() it (not necessarily with this exact syntax
;-)).


On 2013/09/30 19:40:13, Alexei Svitkine wrote:
> Either add all of these flags to the config for the study, or call
> FindFullName() only after you've checked all the flags that are not in the
> config.
> 
> (Calling it marks the trial as "used" and therefore will be reported in
> metrics.)
> 
> Alternatively, for flags that are not in the config, you can call Disable() on
> the FieldTrial object returned by Find() to also disable reporting for that
> trial.

[ramant (doing other things), 2013-09-30 22:28:56]

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

https://codereview.chromium.org/25087007/diff/11001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:720: if (command_line.HasSwitch(switches::kUseSpdy))
{
On 2013/09/30 20:44:29, motek. wrote:
> From the Department of Redundancy Department: pretty much what asvitkine@
wrote.
> 
> 
> Do all these flags affect your experiment in any material way? If not, it
might
> be best to split the chain of ifs into a part affected by the study and the
> rest. 
> 
> You may also want to single out the case where SPDY is disabled by policy and
> FieldTrialList::Find()->Disable() it (not necessarily with this exact syntax
> ;-)).
> 
> 
> On 2013/09/30 19:40:13, Alexei Svitkine wrote:
> > Either add all of these flags to the config for the study, or call
> > FindFullName() only after you've checked all the flags that are not in the
> > config.
> > 
> > (Calling it marks the trial as "used" and therefore will be reported in
> > metrics.)
> > 
> > Alternatively, for flags that are not in the config, you can call Disable()
on
> > the FieldTrial object returned by Find() to also disable reporting for that
> > trial.
> 

Done.

https://codereview.chromium.org/25087007/diff/11001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:720: if (command_line.HasSwitch(switches::kUseSpdy))
{
On 2013/09/30 19:40:13, Alexei Svitkine wrote:
> Either add all of these flags to the config for the study, or call
> FindFullName() only after you've checked all the flags that are not in the
> config.
> 
> (Calling it marks the trial as "used" and therefore will be reported in
> metrics.)
> 
> Alternatively, for flags that are not in the config, you can call Disable() on
> the FieldTrial object returned by Find() to also disable reporting for that
> trial.

Done.

[Alexei Svitkine (slow), 2013-10-01 13:55:30]

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

https://codereview.chromium.org/25087007/diff/16001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:708:
base::FieldTrialList::FindFullName(kSpdyFieldTrialName);
Why is this here?

In your pending config CL, you have a group for kEnableWebSocketOverSpdy. But in
this initialization code, it looks like it doesn't actually enable spdy
explicitly... its still based on the flags below.

So it looks like you should just do this right above the if
(command_line.HasSwitch(switches::kUseSpdy)) line (since you have entries for
all of those groups). Or let me know what I'm missing here...

https://codereview.chromium.org/25087007/diff/16001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:746: base::FieldTrial* trial =
base::FieldTrialList::Find(kSpdyFieldTrialName);
Nit: I'd negate the conditional and put this at the beginning to make this part
clearer.

Alternatively, if you can guarantee that nothing else calls FindFullName() on
this trial, then you can omit this (since it won't be marked active).

[ramant (doing other things), 2013-10-01 17:38:18]

PTAL.

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

https://codereview.chromium.org/25087007/diff/16001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:708:
base::FieldTrialList::FindFullName(kSpdyFieldTrialName);
On 2013/10/01 13:55:31, Alexei Svitkine wrote:
> Why is this here?
> 
> In your pending config CL, you have a group for kEnableWebSocketOverSpdy. But
in
> this initialization code, it looks like it doesn't actually enable spdy
> explicitly... its still based on the flags below.
> 
> So it looks like you should just do this right above the if
> (command_line.HasSwitch(switches::kUseSpdy)) line (since you have entries for
> all of those groups). Or let me know what I'm missing here...

If the command line kEnableWebSocketOverSpdy is selected, I didn't like to
disable SPDY. Reading the code as it is written here, I couldn't assert that
spdy_trial_group wouldn't be kSpdyFieldTrialDisabledGroupName if the above
command line is selected.

The other alternative is to check for
"command_line.HasSwitch(switches::kEnableWebSocketOverSpdy" at line# 738 and if
the command line is set, then default to Spdy3.1.

As per our discussion offline, added the check at line#738.

https://codereview.chromium.org/25087007/diff/16001/chrome/browser/io_thread....
chrome/browser/io_thread.cc:746: base::FieldTrial* trial =
base::FieldTrialList::Find(kSpdyFieldTrialName);
On 2013/10/01 13:55:31, Alexei Svitkine wrote:
> Nit: I'd negate the conditional and put this at the beginning to make this
part
> clearer.
> 
> Alternatively, if you can guarantee that nothing else calls FindFullName() on
> this trial, then you can omit this (since it won't be marked active).

Negated the check.

Done.

[Alexei Svitkine (slow), 2013-10-01 17:41:07]

LGTM

[commit-bot: I haz the power, 2013-10-01 18:06:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/25087007/21001

[ramant (doing other things), 2013-10-01 18:22:01]

adding  Lei Zhang for OWNERS approval.

[commit-bot: I haz the power, 2013-10-01 18:29:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[ramant (doing other things), 2013-10-01 19:04:20]

hakis@: These changes are field trial related to disable SPDY for x% of users.
Could you please take a look at these changes for OWNERS approval? thanks.

[ramant (doing other things), 2013-10-01 19:04:50]

On 2013/10/01 19:04:20, ramant wrote:
> thakis@: These changes are field trial related to disable SPDY for x% of
users.
> Could you please take a look at these changes for OWNERS approval? thanks.

[Nico, 2013-10-01 19:11:38]

lgtm, but remove the "thakis@" line from the Cl description

[commit-bot: I haz the power, 2013-10-01 19:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/25087007/21001

[commit-bot: I haz the power, 2013-10-02 01:49:50]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-02 01:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/25087007/21001

[commit-bot: I haz the power, 2013-10-02 03:24:48]

Change committed as 226389