Fix SimpleCache field trial creation.

chrome/browser/chrome_browser_field_trials.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chrome_browser_field_trials.h"
 
 #include <string>
 
 #include "apps/field_trial_names.h"
 #include "apps/pref_names.h"
@@ skipping 186 matching lines @@
     trial->Disable();
   }
 }
 
 // Sets up the experiment. The actual cache backend choice is made in the net/
 // internals by looking at the experiment state.
 void ChromeBrowserFieldTrials::SetUpSimpleCacheFieldTrial() {
   if (parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
     const std::string opt_value = parsed_command_line_.GetSwitchValueASCII(
         switches::kUseSimpleCacheBackend);
-    if (LowerCaseEqualsASCII(opt_value, "off")) {
-      // This is the default.
-      return;
-    }
     const base::FieldTrial::Probability kDivisor = 100;
     scoped_refptr<base::FieldTrial> trial(
         base::FieldTrialList::FactoryGetFieldTrial("SimpleCacheTrial", kDivisor,
                                                    "No", 2013, 12, 31, NULL));
     trial->UseOneTimeRandomization();
+    if (LowerCaseEqualsASCII(opt_value, "off")) {

[SteveT, 2013/04/17 15:41:02]

Couple things here:

(1) Appending a new group with the same name as the default name is something
that we avoid doing. If you want to force your experiment to be disabled, we
instead recommend appending Yes with weight 0 (which makes the default group 
No" get chosen with 100% probability).

(2) However, it looks like you're dealing with the case where the user is aware
of SimpleCacheBackend, which means that if they opt in or out explicitly, we
don't want them in an experiment that returns data about the feature.

You should instead use a pattern where:

If the user opts in explicitly, tell the feature to be turned on and DON'T make
a FieldTrial.

If the user opts out explicitly, tell the feature to be turned off and DON'T
make a Field Trial.

If the user does not opt in or out, create the field trial with the experimental
weight that you want (kSimpleCacheProbability = 1 on ANDROID, 100 otherwise).

Does this make sense/work for you?

[Alexei Svitkine (slow), 2013/04/17 15:46:52]

Agree with Steve on all points.

Also, I'd like to expand on one aspect - I'm not sure if you're ever planning to
do a server-side config for this trial, but if you are, you should also make
sure to not query the state of the field trial (i.e. not use
FieldTrialList::FindFullName()) if the user explicitly opted in or opted out of
the trial.

[gavinp, 2013/04/17 16:19:42]

There's two considerations that cause me to want everyone in the field trial

(1) We use the field trial name, with FindFullName() to control the feature in
net::. Currently net:: doesn't take command line options or global options, so
this has been the pattern. But, for now, this is what we are doing. This came up
in the initial review of this code.

(2) I'd really really like data on explicit opt in vs opt out, especially since
this feature has no field trial running (no experiments approval yet (hint hint
odean)), and it was absolutely amazing using the field trial to see the results
for the dev channel opt ins already.

So I made an upload that calls the groups "ExplicitFoo" and "ExperimentFoo". Is
this better?

[gavinp, 2013/04/17 16:19:42]

Alexei,

Oh no! Right now FindFullName() is exactly how we turn the cache on and off. Is
there an alternative?

+      trial->AppendGroup("No", 100);
+      return;
+    }
     if (LowerCaseEqualsASCII(opt_value, "on")) {
       trial->AppendGroup("Yes", 100);
       return;
     }
 #if defined(OS_ANDROID)
     if (LowerCaseEqualsASCII(opt_value, "experiment")) {
       // TODO(pasko): Make this the default on Android when the simple cache
       // adds a few more necessary features. Also adjust the probability.
       const base::FieldTrial::Probability kSimpleCacheProbability = 1;
       trial->AppendGroup("Yes", kSimpleCacheProbability);
@@ skipping 50 matching lines @@
   // Call |FindValue()| on the trials below, which may come from the server, to
   // ensure they get marked as "used" for the purposes of data reporting.
   base::FieldTrialList::FindValue("UMA-Dynamic-Binary-Uniformity-Trial");
   base::FieldTrialList::FindValue("UMA-Dynamic-Uniformity-Trial");
   base::FieldTrialList::FindValue("InstantDummy");
   base::FieldTrialList::FindValue("InstantChannel");
   base::FieldTrialList::FindValue("Test0PercentDefault");
   // Activate the autocomplete dynamic field trials.
   OmniboxFieldTrial::ActivateDynamicTrials();
 }