Make it so disabled field trials are not reported as active.

BUG=160310
TEST=New FieldTrialTest.DisabledTrialNotActive test and updated existing tests.

Review URL: https://chromiumcodereview.appspot.com/11359136

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167045 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 51 insertions, 22 deletions

diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index cdefc25..3214f63 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -145,7 +145,8 @@ int FieldTrial::AppendGroup(const std::string& name,
 int FieldTrial::group() {
   FinalizeGroupChoice();
   if (!group_reported_) {
-    FieldTrialList::NotifyFieldTrialGroupSelection(name_, group_name_);
+    if (enable_field_trial_)
+      FieldTrialList::NotifyFieldTrialGroupSelection(name_, group_name_);
     group_reported_ = true;
   }
   return group_;
@@ -209,7 +210,7 @@ void FieldTrial::FinalizeGroupChoice() {
 }
 
 bool FieldTrial::GetActiveGroup(ActiveGroup* active_group) const {
-  if (!group_reported_)
+  if (!group_reported_ || !enable_field_trial_)
     return false;
   DCHECK_NE(group_, kNotFinalized);
   active_group->trial = name_;
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index 764c635..765c50f 100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -217,9 +217,10 @@ class BASE_EXPORT FieldTrial : public RefCounted<FieldTrial> {
 
   // Returns the trial name and selected group name for this field trial via
   // the output parameter |active_group|, but only if the group has already
-  // been chosen and has been externally observed via |group()|. In that case,
-  // true is returned and |active_group| is filled in; otherwise, the result
-  // is false and |active_group| is left untouched.
+  // been chosen and has been externally observed via |group()| and the trial
+  // has not been disabled. In that case, true is returned and |active_group|
+  // is filled in; otherwise, the result is false and |active_group| is left
+  // untouched.
   bool GetActiveGroup(ActiveGroup* active_group) const;
 
   // Returns the group_name. A winner need not have been chosen.
@@ -355,13 +356,15 @@ class BASE_EXPORT FieldTrialList {
   // one process, and secondary processes can be synchronized on the result.
   // The resulting string contains the name and group name pairs of all
   // registered FieldTrials for which the group has been chosen and externally
-  // observed (via |group()|), with "/" used to separate all names and to
-  // terminate the string. This string is parsed by |CreateTrialsFromString()|.
+  // observed (via |group()|) and which have not been disabled, with "/" used
+  // to separate all names and to terminate the string. This string is parsed
+  // by |CreateTrialsFromString()|.
   static void StatesToString(std::string* output);
 
   // Fills in the supplied vector |active_groups| (which must be empty when
   // called) with a snapshot of all registered FieldTrials for which the group
-  // has been chosen and externally observed (via |group()|).
+  // has been chosen and externally observed (via |group()|) and which have
+  // not been disabled.
   static void GetActiveFieldTrialGroups(
       FieldTrial::ActiveGroups* active_groups);
 
diff --git a/base/metrics/field_trial_unittest.cc b/base/metrics/field_trial_unittest.cc
index 9683274..673e6b6 100644
--- a/base/metrics/field_trial_unittest.cc
+++ b/base/metrics/field_trial_unittest.cc
@@ -388,8 +388,8 @@ TEST_F(FieldTrialTest, Save) {
 }
 
 TEST_F(FieldTrialTest, Restore) {
-  EXPECT_TRUE(FieldTrialList::Find("Some_name") == NULL);
-  EXPECT_TRUE(FieldTrialList::Find("xxx") == NULL);
+  ASSERT_FALSE(FieldTrialList::TrialExists("Some_name"));
+  ASSERT_FALSE(FieldTrialList::TrialExists("xxx"));
 
   FieldTrialList::CreateTrialsFromString("Some_name/Winner/xxx/yyyy/");
 
@@ -431,7 +431,7 @@ TEST_F(FieldTrialTest, DuplicateRestore) {
 }
 
 TEST_F(FieldTrialTest, CreateFieldTrial) {
-  EXPECT_TRUE(FieldTrialList::Find("Some_name") == NULL);
+  ASSERT_FALSE(FieldTrialList::TrialExists("Some_name"));
 
   FieldTrialList::CreateFieldTrial("Some_name", "Winner");
 
@@ -577,48 +577,73 @@ TEST_F(FieldTrialTest, ObserveDisabled) {
   const char kTrialName[] = "TrialToObserve2";
 
   TestFieldTrialObserver observer;
+  int default_group = -1;
   FieldTrial* trial =
       FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
-                                           next_year_, 12, 31, NULL);
+                                           next_year_, 12, 31, &default_group);
   trial->AppendGroup("A", 25);
   trial->AppendGroup("B", 25);
   trial->AppendGroup("C", 25);
   trial->Disable();
 
-  // Observer shouldn't be notified until group() is called.
+  // Observer shouldn't be notified of a disabled trial.
   message_loop_.RunAllPending();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 
-  trial->group();
+  // Observer shouldn't be notified even after a |group()| call.
+  EXPECT_EQ(default_group, trial->group());
   message_loop_.RunAllPending();
-  EXPECT_EQ(kTrialName, observer.trial_name());
-  EXPECT_EQ(kDefaultGroupName, observer.group_name());
+  EXPECT_TRUE(observer.trial_name().empty());
+  EXPECT_TRUE(observer.group_name().empty());
 }
 
 TEST_F(FieldTrialTest, ObserveForcedDisabled) {
   const char kTrialName[] = "TrialToObserve3";
 
   TestFieldTrialObserver observer;
+  int default_group = -1;
   FieldTrial* trial =
       FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
-                                           next_year_, 12, 31, NULL);
+                                           next_year_, 12, 31, &default_group);
   trial->AppendGroup("A", 25);
   trial->AppendGroup("B", 25);
   trial->AppendGroup("C", 25);
   trial->SetForced();
   trial->Disable();
 
-  // Observer shouldn't be notified until group() is called, even if SetForced()
-  // was called.
+  // Observer shouldn't be notified of a disabled trial, even when forced.
   message_loop_.RunAllPending();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 
-  trial->group();
+  // Observer shouldn't be notified even after a |group()| call.
+  EXPECT_EQ(default_group, trial->group());
   message_loop_.RunAllPending();
-  EXPECT_EQ(kTrialName, observer.trial_name());
-  EXPECT_EQ(kDefaultGroupName, observer.group_name());
+  EXPECT_TRUE(observer.trial_name().empty());
+  EXPECT_TRUE(observer.group_name().empty());
 }
 
+TEST_F(FieldTrialTest, DisabledTrialNotActive) {
+  const char kTrialName[] = "DisabledTrial";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+
+  FieldTrial* trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
+                                           next_year_, 12, 31, NULL);
+  trial->AppendGroup("X", 50);
+  trial->Disable();
+
+  // Ensure the trial is not listed as active.
+  FieldTrial::ActiveGroups active_groups;
+  FieldTrialList::GetActiveFieldTrialGroups(&active_groups);
+  EXPECT_TRUE(active_groups.empty());
+
+  // Ensure the trial is not listed in the |StatesToString()| result.
+  std::string states;
+  FieldTrialList::StatesToString(&states);
+  EXPECT_TRUE(states.empty());
+}
+
+
 }  // namespace base