Restrict Instant field trial to UMA opt-in users.

chrome/browser/instant/instant_field_trial.cc

Index: chrome/browser/instant/instant_field_trial.cc
diff --git a/chrome/browser/instant/instant_field_trial.cc b/chrome/browser/instant/instant_field_trial.cc
index 90016ff3003085d050cc50efbc519781a21256b9..754ca43a22913d9387061c5fc92f0b4277216d24 100644
--- a/chrome/browser/instant/instant_field_trial.cc
+++ b/chrome/browser/instant/instant_field_trial.cc
@@ -4,24 +4,40 @@
 
 #include "chrome/browser/instant/instant_field_trial.h"
 
+#include "base/metrics/field_trial.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 
 namespace {
-bool field_trial_active_ = false;
+
+// Field trial IDs of the control and experiment groups. Though they are not
+// literally "const", they are set only once, in Activate() below.
+int kControlGroupId1 = 0;

[jar (doing other things), 2011/08/08 22:28:29]

As you noted, these are not const, and hence should not have a k prefix.

In addition, they appear to be global (to the file/namespace), so they should
have a g_ prefix.

Alternatively, you might want to make them static member variables, or if there
is only one instance of InstantFieldTrial, make them member variables.

[sreeram, 2011/08/08 22:42:23]

Done. These are unnecessary implementation details that I don't want to expose
in the interface, so I'd rather not have them as instance or static members. So,
I've changed the names to be g_*.

+int kControlGroupId2 = 0;
+int kExperimentGroupId1 = 0;
+int kExperimentGroupId2 = 0;
+
 }
 
 // static
 void InstantFieldTrial::Activate() {
-  field_trial_active_ = true;
+  scoped_refptr<base::FieldTrial> trial(
+      new base::FieldTrial("Instant", 10000, "InstantInactive", 2012, 1, 1));
+
+  // One-time randomization is disabled if the user is not opted into UMA.
+  if (!base::FieldTrialList::IsOneTimeRandomizationEnabled())
+    return;
+  trial->UseOneTimeRandomization();
+
+  kControlGroupId1 = trial->AppendGroup("InstantControl1", 4500);  // 45%
+  kControlGroupId2 = trial->AppendGroup("InstantControl2", 4500);  // 45%
+  kExperimentGroupId1 = trial->AppendGroup("InstantExperiment1", 500);  // 5%

[jar (doing other things), 2011/08/08 22:28:29]

Notice that you quite reasonably only use probabilities as low as 5%.  As a
result, I really don't think you need more than 1000 distinct
one-time-randomization values (which would facilitate 0.1% granularity in
experiments).  As I said, I'm generally in favor of less user tagging when
possible, to avoid any possible privacy concerns.

[sreeram, 2011/08/08 22:42:23]

Done. I've reduced the granularity to 0.1%, but note that even if we had the
divisor as a million or billion, it wouldn't affect privacy. The random number
is never stored in preferences, unlike the previous code (which was not
restricted to UMA users only).

[jar (doing other things), 2011/08/09 16:34:28]

ah... you are correct.  My mistake.  Thanks!

+  kExperimentGroupId2 = trial->AppendGroup("InstantExperiment2", 500);  // 5%
 }
 
 // static
 InstantFieldTrial::Group InstantFieldTrial::GetGroup(Profile* profile) {
-  if (!field_trial_active_)
-    return INACTIVE;
-
   if (profile->IsOffTheRecord())
     return INACTIVE;
 
@@ -33,11 +49,18 @@ InstantFieldTrial::Group InstantFieldTrial::GetGroup(Profile* profile) {
     return INACTIVE;
   }
 
-  const int random = prefs->GetInteger(prefs::kInstantFieldTrialRandomDraw);
-  return random < 4500 ? CONTROL1 :    // 45%
-         random < 9000 ? CONTROL2 :    // 45%
-         random < 9500 ? EXPERIMENT1   //  5%
-                       : EXPERIMENT2;  //  5%
+  const int group = base::FieldTrialList::FindValue("Instant");
+
+  if (group == base::FieldTrial::kNotFinalized ||
+      group == base::FieldTrial::kDefaultGroupNumber) {
+    return INACTIVE;
+  }
+
+  return group == kControlGroupId1    ? CONTROL1 :
+         group == kControlGroupId2    ? CONTROL2 :
+         group == kExperimentGroupId1 ? EXPERIMENT1 :
+         group == kExperimentGroupId2 ? EXPERIMENT2
+                                      : INACTIVE;
 }
 
 // static