Make FieldTrialList::StatesToString() only report active trials.

This change makes StatesToString() use GetActiveFieldTrialGroups(),
making the two function have the same behavior.

With this change, group() must be called on trials that are to be synchronized
to sub-processes via the initial command line (which was already true for later
syncs via the observer child process notification). This also actually fixes a
subtle bug that would cause inconsistent behavior when group() wasn't called
in the old code.

In the previous code, if group() was not called, then if a group other than the
default was chosen, it would be reflected in the StatesToString() state, but if
the default group was the remaining one and group() wasn't called, its state
would not be reflected in the string.

With the change, it is now consistent - for any state to be reflected in
StatesToString(), group() needs to be called. I've expanded the test to check
the default group case that was inconsistent (not working) before.

This does mean that any trials that should be visible to the renderer must
ensure to call group() in the browser process, but I'm not sure whether
there are any such trials that don't do this currently - hoping the reviewers
could chime in here. Thanks! 

BUG=158801
TEST=Expanded unit tests in field_trial_unittest.cc.

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

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

3 files changed, 29 insertions, 19 deletions

diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index f048897..8ca4f1c 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -320,22 +320,15 @@ bool FieldTrialList::TrialExists(const std::string& name) {
 
 // static
 void FieldTrialList::StatesToString(std::string* output) {
-  DCHECK(output->empty());
-  if (!global_)
-    return;
-  AutoLock auto_lock(global_->lock_);
-
-  for (RegistrationList::iterator it = global_->registered_.begin();
-       it != global_->registered_.end(); ++it) {
-    const std::string& name = it->first;
-    std::string group_name = it->second->group_name_internal();
-    if (group_name.empty())
-      continue;  // Should not include uninitialized trials.
-    DCHECK_EQ(name.find(kPersistentStringSeparator), std::string::npos);
-    DCHECK_EQ(group_name.find(kPersistentStringSeparator), std::string::npos);
-    output->append(name);
+  FieldTrial::ActiveGroups active_groups;
+  GetActiveFieldTrialGroups(&active_groups);
+  for (FieldTrial::ActiveGroups::const_iterator it = active_groups.begin();
+       it != active_groups.end(); ++it) {
+    DCHECK_EQ(std::string::npos, it->trial.find(kPersistentStringSeparator));
+    DCHECK_EQ(std::string::npos, it->group.find(kPersistentStringSeparator));
+    output->append(it->trial);
     output->append(1, kPersistentStringSeparator);
-    output->append(group_name);
+    output->append(it->group);
     output->append(1, kPersistentStringSeparator);
   }
 }
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index 81d4d3d..9018850 100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -350,12 +350,13 @@ class BASE_EXPORT FieldTrialList {
   // Returns true if the named trial has been registered.
   static bool TrialExists(const std::string& name);
 
-  // Creates a persistent representation of all FieldTrial instances for
+  // Creates a persistent representation of active FieldTrial instances for
   // resurrection in another process. This allows randomization to be done in
   // one process, and secondary processes can be synchronized on the result.
-  // The resulting string contains the name and group name pairs for all trials,
-  // with "/" used to separate all names and to terminate the string. This
-  // string is parsed by CreateTrialsFromString().
+  // 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()|.
   static void StatesToString(std::string* output);
 
   // Fills in the supplied vector |active_groups| (which must be empty when
diff --git a/base/metrics/field_trial_unittest.cc b/base/metrics/field_trial_unittest.cc
index 78ecfe2..9683274 100644
--- a/base/metrics/field_trial_unittest.cc
+++ b/base/metrics/field_trial_unittest.cc
@@ -359,6 +359,8 @@ TEST_F(FieldTrialTest, Save) {
 
   // Create a winning group.
   trial->AppendGroup("Winner", 10);
+  // Finalize the group selection by accessing the selected group.
+  trial->group();
   FieldTrialList::StatesToString(&save_string);
   EXPECT_EQ("Some name/Winner/", save_string);
   save_string.clear();
@@ -367,10 +369,22 @@ TEST_F(FieldTrialTest, Save) {
   FieldTrial* trial2 = FieldTrialList::FactoryGetFieldTrial(
       "xxx", 10, "Default xxx", next_year_, 12, 31, NULL);
   trial2->AppendGroup("yyyy", 10);
+  // Finalize the group selection by accessing the selected group.
+  trial2->group();
 
   FieldTrialList::StatesToString(&save_string);
   // We assume names are alphabetized... though this is not critical.
   EXPECT_EQ("Some name/Winner/xxx/yyyy/", save_string);
+  save_string.clear();
+
+  // Create a third trial with only the default group.
+  FieldTrial* trial3 = FieldTrialList::FactoryGetFieldTrial(
+      "zzz", 10, "default", next_year_, 12, 31, NULL);
+  // Finalize the group selection by accessing the selected group.
+  trial3->group();
+
+  FieldTrialList::StatesToString(&save_string);
+  EXPECT_EQ("Some name/Winner/xxx/yyyy/zzz/default/", save_string);
 }
 
 TEST_F(FieldTrialTest, Restore) {
@@ -403,6 +417,8 @@ TEST_F(FieldTrialTest, DuplicateRestore) {
   FieldTrial* trial = FieldTrialList::FactoryGetFieldTrial(
       "Some name", 10, "Default some name", next_year_, 12, 31, NULL);
   trial->AppendGroup("Winner", 10);
+  // Finalize the group selection by accessing the selected group.
+  trial->group();
   std::string save_string;
   FieldTrialList::StatesToString(&save_string);
   EXPECT_EQ("Some name/Winner/", save_string);