Ensure that field trials synced to the renderer process are considered "active".

http://crrev.com/166168 made trials on which group() hasn't been called not get
reported. This resulted in renderer crash reports not being tagged with field
trials because group() wasn't called on the renderer side after they are
synchronized from the browser process.

This change makes CreateTrialsFromString() mark trials as used as well as
making trials synchronized to the renderer later via OnSetFieldTrialGroup()
notification also get marked as used.

Also fixes a couple of lint warnings in chrome_render_process_observer.cc and
changes field_trial_unittest.cc to not use a deprecated function.

BUG=158801
TEST=New FieldTrialTest.CreateFieldTrialIsActive test.

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

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

4 files changed, 61 insertions, 12 deletions

diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 3214f63..3a563aa 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -369,8 +369,13 @@ bool FieldTrialList::CreateTrialsFromString(const std::string& trials_string) {
                            group_name_end - name_end - 1);
     next_item = group_name_end + 1;
 
-    if (!CreateFieldTrial(name, group_name))
+    FieldTrial* trial = CreateFieldTrial(name, group_name);
+    if (!trial)
       return false;
+    // Call |group()| to mark the trial as "used" and notify observers, if any.
+    // This is needed to ensure the trial is properly reported in child process
+    // crash reports.
+    trial->group();
   }
   return true;
 }
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index 765c50f..8e7a2efe 100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -369,11 +369,12 @@ class BASE_EXPORT FieldTrialList {
       FieldTrial::ActiveGroups* active_groups);
 
   // Use a state string (re: StatesToString()) to augment the current list of
-  // field tests to include the supplied tests, and using a 100% probability for
-  // each test, force them to have the same group string.  This is commonly used
-  // in a non-browser process, to carry randomly selected state in a browser
-  // process into this non-browser process, but could also be invoked through a
-  // command line argument to the browser process.
+  // field trials to include the supplied trials, and using a 100% probability
+  // for each trial, force them to have the same group string. This is commonly
+  // used in a non-browser process, to carry randomly selected state in a
+  // browser process into this non-browser process, but could also be invoked
+  // through a command line argument to the browser process. The created field
+  // trials are marked as "used" for the purposes of active trial reporting.
   static bool CreateTrialsFromString(const std::string& prior_trials);
 
   // Create a FieldTrial with the given |name| and using 100% probability for
diff --git a/base/metrics/field_trial_unittest.cc b/base/metrics/field_trial_unittest.cc
index 673e6b6..1ab8467 100644
--- a/base/metrics/field_trial_unittest.cc
+++ b/base/metrics/field_trial_unittest.cc
@@ -430,6 +430,31 @@ TEST_F(FieldTrialTest, DuplicateRestore) {
   EXPECT_FALSE(FieldTrialList::CreateTrialsFromString("Some name/Loser/"));
 }
 
+TEST_F(FieldTrialTest, CreateTrialsFromStringAreActive) {
+  ASSERT_FALSE(FieldTrialList::TrialExists("Abc"));
+  ASSERT_FALSE(FieldTrialList::TrialExists("Xyz"));
+  ASSERT_TRUE(FieldTrialList::CreateTrialsFromString("Abc/def/Xyz/zyx/"));
+
+  FieldTrial::ActiveGroups active_groups;
+  FieldTrialList::GetActiveFieldTrialGroups(&active_groups);
+  ASSERT_EQ(2U, active_groups.size());
+  EXPECT_EQ("Abc", active_groups[0].trial);
+  EXPECT_EQ("def", active_groups[0].group);
+  EXPECT_EQ("Xyz", active_groups[1].trial);
+  EXPECT_EQ("zyx", active_groups[1].group);
+}
+
+TEST_F(FieldTrialTest, CreateTrialsFromStringObserver) {
+  ASSERT_FALSE(FieldTrialList::TrialExists("Abc"));
+
+  TestFieldTrialObserver observer;
+  ASSERT_TRUE(FieldTrialList::CreateTrialsFromString("Abc/def/"));
+
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ("Abc", observer.trial_name());
+  EXPECT_EQ("def", observer.group_name());
+}
+
 TEST_F(FieldTrialTest, CreateFieldTrial) {
   ASSERT_FALSE(FieldTrialList::TrialExists("Some_name"));
 
@@ -441,6 +466,17 @@ TEST_F(FieldTrialTest, CreateFieldTrial) {
   EXPECT_EQ("Some_name", trial->name());
 }
 
+TEST_F(FieldTrialTest, CreateFieldTrialIsNotActive) {
+  const char kTrialName[] = "CreateFieldTrialIsActiveTrial";
+  const char kWinnerGroup[] = "Winner";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+  FieldTrialList::CreateFieldTrial(kTrialName, kWinnerGroup);
+
+  FieldTrial::ActiveGroups active_groups;
+  FieldTrialList::GetActiveFieldTrialGroups(&active_groups);
+  EXPECT_TRUE(active_groups.empty());
+}
+
 TEST_F(FieldTrialTest, DuplicateFieldTrial) {
   FieldTrial* trial = FieldTrialList::FactoryGetFieldTrial(
       "Some_name", 10, "Default some name", next_year_, 12, 31, NULL);
@@ -565,7 +601,7 @@ TEST_F(FieldTrialTest, Observe) {
   const int chosen_group = trial->group();
   EXPECT_TRUE(chosen_group == default_group || chosen_group == secondary_group);
 
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
   EXPECT_EQ(kTrialName, observer.trial_name());
   if (chosen_group == default_group)
     EXPECT_EQ(kDefaultGroupName, observer.group_name());
@@ -587,13 +623,13 @@ TEST_F(FieldTrialTest, ObserveDisabled) {
   trial->Disable();
 
   // Observer shouldn't be notified of a disabled trial.
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 
   // Observer shouldn't be notified even after a |group()| call.
   EXPECT_EQ(default_group, trial->group());
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 }
@@ -613,13 +649,13 @@ TEST_F(FieldTrialTest, ObserveForcedDisabled) {
   trial->Disable();
 
   // Observer shouldn't be notified of a disabled trial, even when forced.
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 
   // Observer shouldn't be notified even after a |group()| call.
   EXPECT_EQ(default_group, trial->group());
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
   EXPECT_TRUE(observer.trial_name().empty());
   EXPECT_TRUE(observer.group_name().empty());
 }
diff --git a/chrome/renderer/chrome_render_process_observer.cc b/chrome/renderer/chrome_render_process_observer.cc
index 9b84e40..98517e2 100644
--- a/chrome/renderer/chrome_render_process_observer.cc
+++ b/chrome/renderer/chrome_render_process_observer.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/renderer/chrome_render_process_observer.h"
 
+#include <limits>
+#include <vector>
+
 #include "base/allocator/allocator_extension.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -269,7 +272,11 @@ void ChromeRenderProcessObserver::OnGetCacheResourceStats() {
 void ChromeRenderProcessObserver::OnSetFieldTrialGroup(
     const std::string& field_trial_name,
     const std::string& group_name) {
-  base::FieldTrialList::CreateFieldTrial(field_trial_name, group_name);
+  base::FieldTrial* trial =
+      base::FieldTrialList::CreateFieldTrial(field_trial_name, group_name);
+  // Ensure the trial is marked as "used" by calling group() on it. This is
+  // needed to ensure the trial is properly reported in renderer crash reports.
+  trial->group();
   chrome_variations::SetChildProcessLoggingVariationList();
 }