Have MakeName prepend the underscore to the field trial group name,

rather than having the field trial group name itself include the
underscore.

This will change the histogram names for only one field trial,
the CacheSizeGroup field trial, and I have got in touch with Mike
and Ricardo (who wrote that code); they confirmed that they don't
mind the name change.

Also updates the field trial unit tests to reflect the change,
and to make the use of the expected/actual arguments to the
unit test macros consistent

TEST=Unit tests pass.
BUG=None.
Review URL: http://codereview.chromium.org/3066036

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

3 files changed, 35 insertions, 25 deletions

diff --git a/base/field_trial.cc b/base/field_trial.cc
index 137c8c3..84e96a6 100644
--- a/base/field_trial.cc
+++ b/base/field_trial.cc
@@ -19,6 +19,8 @@ const int FieldTrial::kAllRemainingProbability = -2;
 // static
 const char FieldTrialList::kPersistentStringSeparator('/');
 
+static const char kHistogramFieldTrialSeparator('_');
+
 //------------------------------------------------------------------------------
 // FieldTrial methods and members.
 
@@ -47,7 +49,7 @@ int FieldTrial::AppendGroup(const std::string& name,
     // This is the group that crossed the random line, so we do the assignment.
     group_ = next_group_number_;
     if (name.empty())
-      StringAppendF(&group_name_, "_%d", group_);
+      StringAppendF(&group_name_, "%d", group_);
     else
       group_name_ = name;
   }
@@ -58,6 +60,7 @@ int FieldTrial::AppendGroup(const std::string& name,
 std::string FieldTrial::MakeName(const std::string& name_prefix,
                                  const std::string& trial_name) {
   std::string big_string(name_prefix);
+  big_string.append(1, kHistogramFieldTrialSeparator);
   return big_string.append(FieldTrialList::FindFullName(trial_name));
 }
 
diff --git a/base/field_trial.h b/base/field_trial.h
index 9dba639..b1f0a07 100644
--- a/base/field_trial.h
+++ b/base/field_trial.h
@@ -34,8 +34,8 @@
 // // process teardown, courtesy of their automatic registration in
 // // FieldTrialList.
 // scoped_refptr<FieldTrial> trial = new FieldTrial("MemoryExperiment", 1000);
-// int group1 = trial->AppendGroup("_high_mem", 20);  // 2% in _high_mem group.
-// int group2 = trial->AppendGroup("_low_mem", 20);   // 2% in _low_mem group.
+// int group1 = trial->AppendGroup("high_mem", 20);  // 2% in high_mem group.
+// int group2 = trial->AppendGroup("low_mem", 20);   // 2% in low_mem group.
 // // Take action depending of which group we randomly land in.
 // if (trial->group() == group1)
 //   SetPruningAlgorithm(kType1);  // Sample setting of browser state.
@@ -125,7 +125,7 @@ class FieldTrial : public base::RefCounted<FieldTrial> {
   // This is empty of the trial is not in the experiment.
   const std::string name_;
 
-  // The maximu sum of all probabilities supplied, which corresponds to 100%.
+  // The maximum sum of all probabilities supplied, which corresponds to 100%.
   // This is the scaling factor used to adjust supplied probabilities.
   Probability divisor_;
 
diff --git a/base/field_trial_unittest.cc b/base/field_trial_unittest.cc
index 671376d..7d2ea18 100644
--- a/base/field_trial_unittest.cc
+++ b/base/field_trial_unittest.cc
@@ -26,9 +26,9 @@ TEST_F(FieldTrialTest, Registration) {
   EXPECT_FALSE(FieldTrialList::Find(name2));
 
   FieldTrial* trial1 = new FieldTrial(name1, 10);
-  EXPECT_EQ(trial1->group(), FieldTrial::kNotParticipating);
-  EXPECT_EQ(trial1->name(), name1);
-  EXPECT_EQ(trial1->group_name(), "");
+  EXPECT_EQ(FieldTrial::kNotParticipating, trial1->group());
+  EXPECT_EQ(name1, trial1->name());
+  EXPECT_EQ("", trial1->group_name());
 
   trial1->AppendGroup("", 7);
 
@@ -36,9 +36,9 @@ TEST_F(FieldTrialTest, Registration) {
   EXPECT_FALSE(FieldTrialList::Find(name2));
 
   FieldTrial* trial2 = new FieldTrial(name2, 10);
-  EXPECT_EQ(trial2->group(), FieldTrial::kNotParticipating);
-  EXPECT_EQ(trial2->name(), name2);
-  EXPECT_EQ(trial2->group_name(), "");
+  EXPECT_EQ(FieldTrial::kNotParticipating, trial2->group());
+  EXPECT_EQ(name2, trial2->name());
+  EXPECT_EQ("", trial2->group_name());
 
   trial2->AppendGroup("a first group", 7);
 
@@ -56,16 +56,16 @@ TEST_F(FieldTrialTest, AbsoluteProbabilities) {
     always_false[0] = i;
 
     FieldTrial* trial_true = new FieldTrial(always_true, 10);
-    const std::string winner = "_TheWinner";
+    const std::string winner = "TheWinner";
     int winner_group = trial_true->AppendGroup(winner, 10);
 
-    EXPECT_EQ(trial_true->group(), winner_group);
-    EXPECT_EQ(trial_true->group_name(), winner);
+    EXPECT_EQ(winner_group, trial_true->group());
+    EXPECT_EQ(winner, trial_true->group_name());
 
     FieldTrial* trial_false = new FieldTrial(always_false, 10);
     int loser_group = trial_false->AppendGroup("ALoser", 0);
 
-    EXPECT_NE(trial_false->group(), loser_group);
+    EXPECT_NE(loser_group, trial_false->group());
   }
 }
 
@@ -123,15 +123,15 @@ TEST_F(FieldTrialTest, OneWinner) {
     int might_win = trial->AppendGroup("", 1);
 
     if (trial->group() == might_win) {
-      EXPECT_EQ(winner_index, -2);
+      EXPECT_EQ(-2, winner_index);
       winner_index = might_win;
-      StringAppendF(&winner_name, "_%d", might_win);
+      StringAppendF(&winner_name, "%d", might_win);
       EXPECT_EQ(winner_name, trial->group_name());
     }
   }
   EXPECT_GE(winner_index, 0);
   EXPECT_EQ(trial->group(), winner_index);
-  EXPECT_EQ(winner_name, trial->group_name());
+  EXPECT_EQ(trial->group_name(), winner_name);
 }
 
 TEST_F(FieldTrialTest, Save) {
@@ -139,15 +139,15 @@ TEST_F(FieldTrialTest, Save) {
 
   FieldTrial* trial = new FieldTrial("Some name", 10);
   // There is no winner yet, so no textual group name is associated with trial.
-  EXPECT_EQ(trial->group_name(), "");
+  EXPECT_EQ("", trial->group_name());
   FieldTrialList::StatesToString(&save_string);
-  EXPECT_EQ(save_string, "");
+  EXPECT_EQ("", save_string);
   save_string.clear();
 
   // Create a winning group.
   trial->AppendGroup("Winner", 10);
   FieldTrialList::StatesToString(&save_string);
-  EXPECT_EQ(save_string, "Some name/Winner/");
+  EXPECT_EQ("Some name/Winner/", save_string);
   save_string.clear();
 
   // Create a second trial and winning group.
@@ -156,7 +156,7 @@ TEST_F(FieldTrialTest, Save) {
 
   FieldTrialList::StatesToString(&save_string);
   // We assume names are alphabetized... though this is not critical.
-  EXPECT_EQ(save_string, "Some name/Winner/xxx/yyyy/");
+  EXPECT_EQ("Some name/Winner/xxx/yyyy/", save_string);
 }
 
 TEST_F(FieldTrialTest, Restore) {
@@ -167,13 +167,13 @@ TEST_F(FieldTrialTest, Restore) {
 
   FieldTrial* trial = FieldTrialList::Find("Some_name");
   ASSERT_NE(static_cast<FieldTrial*>(NULL), trial);
-  EXPECT_EQ(trial->group_name(), "Winner");
-  EXPECT_EQ(trial->name(), "Some_name");
+  EXPECT_EQ("Winner", trial->group_name());
+  EXPECT_EQ("Some_name", trial->name());
 
   trial = FieldTrialList::Find("xxx");
   ASSERT_NE(static_cast<FieldTrial*>(NULL), trial);
-  EXPECT_EQ(trial->group_name(), "yyyy");
-  EXPECT_EQ(trial->name(), "xxx");
+  EXPECT_EQ("yyyy", trial->group_name());
+  EXPECT_EQ("xxx", trial->name());
 }
 
 TEST_F(FieldTrialTest, BogusRestore) {
@@ -196,3 +196,10 @@ TEST_F(FieldTrialTest, DuplicateRestore) {
   // But it is an error to try to change to a different winner.
   EXPECT_FALSE(FieldTrialList::StringAugmentsState("Some name/Loser/"));
 }
+
+TEST_F(FieldTrialTest, MakeName) {
+  FieldTrial* trial = new FieldTrial("Field Trial", 10);
+  trial->AppendGroup("Winner", 10);
+  EXPECT_EQ("Histogram_Winner",
+            FieldTrial::MakeName("Histogram", "Field Trial"));
+}