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

7 files changed, 60 insertions, 50 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"));
+}
diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index f9d9398c..1f95c1f5 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -187,19 +187,19 @@ void BrowserMainParts::ConnectionFieldTrial() {
   scoped_refptr<FieldTrial> connect_trial =
       new FieldTrial("ConnCountImpact", kConnectDivisor);
 
-  const int connect_5 = connect_trial->AppendGroup("_conn_count_5",
+  const int connect_5 = connect_trial->AppendGroup("conn_count_5",
                                                    kConnectProbability);
-  const int connect_7 = connect_trial->AppendGroup("_conn_count_7",
+  const int connect_7 = connect_trial->AppendGroup("conn_count_7",
                                                    kConnectProbability);
-  const int connect_8 = connect_trial->AppendGroup("_conn_count_8",
+  const int connect_8 = connect_trial->AppendGroup("conn_count_8",
                                                    kConnectProbability);
-  const int connect_9 = connect_trial->AppendGroup("_conn_count_9",
+  const int connect_9 = connect_trial->AppendGroup("conn_count_9",
                                                    kConnectProbability);
   // This (6) is the current default value. Having this group declared here
   // makes it straightforward to modify |kConnectProbability| such that the same
   // probability value will be assigned to all the other groups, while
   // preserving the remainder of the of probability space to the default value.
-  const int connect_6 = connect_trial->AppendGroup("_conn_count_6",
+  const int connect_6 = connect_trial->AppendGroup("conn_count_6",
       FieldTrial::kAllRemainingProbability);
 
   const int connect_trial_group = connect_trial->group();
@@ -233,13 +233,13 @@ void BrowserMainParts::SocketTimeoutFieldTrial() {
       new FieldTrial("IdleSktToImpact", kIdleSocketTimeoutDivisor);
 
   const int socket_timeout_5 =
-      socket_timeout_trial->AppendGroup("_idle_timeout_5",
+      socket_timeout_trial->AppendGroup("idle_timeout_5",
                                         kSocketTimeoutProbability);
   const int socket_timeout_20 =
-      socket_timeout_trial->AppendGroup("_idle_timeout_20",
+      socket_timeout_trial->AppendGroup("idle_timeout_20",
                                         kSocketTimeoutProbability);
   const int socket_timeout_60 =
-      socket_timeout_trial->AppendGroup("_idle_timeout_60",
+      socket_timeout_trial->AppendGroup("idle_timeout_60",
                                         kSocketTimeoutProbability);
   // This (10 seconds) is the current default value. Declaring it at the end
   // allows for assigning the remainder of the probability space to it; which
@@ -247,7 +247,7 @@ void BrowserMainParts::SocketTimeoutFieldTrial() {
   // down the road if we see the need to, while the remaining groups are
   // are assigned an equal share of the probability space.
   const int socket_timeout_10 =
-      socket_timeout_trial->AppendGroup("_idle_timeout_10",
+      socket_timeout_trial->AppendGroup("idle_timeout_10",
                                         FieldTrial::kAllRemainingProbability);
 
   const int idle_to_trial_group = socket_timeout_trial->group();
@@ -336,10 +336,10 @@ void BrowserMainParts::SpdyFieldTrial() {
         new FieldTrial("SpdyImpact", kSpdyDivisor);
     // npn with only http support, no spdy.
     int npn_http_grp =
-        trial->AppendGroup("_npn_with_http", npnhttp_probability);
+        trial->AppendGroup("npn_with_http", npnhttp_probability);
     // npn with spdy support.
     int npn_spdy_grp =
-        trial->AppendGroup("_npn_with_spdy", npnspdy_probability);
+        trial->AppendGroup("npn_with_spdy", npnspdy_probability);
     int trial_grp = trial->group();
     if (trial_grp == npn_http_grp) {
       is_spdy_trial = true;
@@ -1262,9 +1262,9 @@ int BrowserMain(const MainFunctionParams& parameters) {
     sdch_supported_domain =
         parsed_command_line.GetSwitchValueASCII(switches::kSdchFilter);
   } else {
-    sdch_trial->AppendGroup("_global_disable_sdch",
+    sdch_trial->AppendGroup("global_disable_sdch",
                             kSDCH_DISABLE_PROBABILITY);
-    int sdch_enabled = sdch_trial->AppendGroup("_global_enable_sdch",
+    int sdch_enabled = sdch_trial->AppendGroup("global_enable_sdch",
         FieldTrial::kAllRemainingProbability);
     if (sdch_enabled != sdch_trial->group())
       sdch_supported_domain = "never_enabled_sdch_for_any_domain";
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index a39b35b..3d1adb3 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -69,9 +69,9 @@ net::HostResolver* CreateGlobalHostResolver() {
         const FieldTrial::Probability kDivisor = 100;
         const FieldTrial::Probability kProbability = 50;  // 50% probability.
         FieldTrial* trial = new FieldTrial("IPv6_Probe", kDivisor);
-        int skip_group = trial->AppendGroup("_IPv6_probe_skipped",
+        int skip_group = trial->AppendGroup("IPv6_probe_skipped",
                                             kProbability);
-        trial->AppendGroup("_IPv6_probe_done",
+        trial->AppendGroup("IPv6_probe_done",
                            FieldTrial::kAllRemainingProbability);
         use_ipv6_probe = (trial->group() != skip_group);
 #endif
diff --git a/chrome/browser/net/predictor_api.cc b/chrome/browser/net/predictor_api.cc
index 98febfe..28e89f2 100644
--- a/chrome/browser/net/predictor_api.cc
+++ b/chrome/browser/net/predictor_api.cc
@@ -603,7 +603,7 @@ PredictorInit::PredictorInit(PrefService* user_prefs,
   trial_ = new FieldTrial("DnsImpact", kDivisor);
 
   // First option is to disable prefetching completely.
-  int disabled_prefetch = trial_->AppendGroup("_disabled_prefetch",
+  int disabled_prefetch = trial_->AppendGroup("disabled_prefetch",
                                               kProbabilityPerGroup);
 
 
@@ -615,27 +615,27 @@ PredictorInit::PredictorInit(PrefService* user_prefs,
   // Experiment 1:
   // Set congestion detection at 250, 500, or 750ms, rather than the 1 second
   // default.
-  int max_250ms_prefetch = trial_->AppendGroup("_max_250ms_queue_prefetch",
+  int max_250ms_prefetch = trial_->AppendGroup("max_250ms_queue_prefetch",
                                                kProbabilityPerGroup);
-  int max_500ms_prefetch = trial_->AppendGroup("_max_500ms_queue_prefetch",
+  int max_500ms_prefetch = trial_->AppendGroup("max_500ms_queue_prefetch",
                                                kProbabilityPerGroup);
-  int max_750ms_prefetch = trial_->AppendGroup("_max_750ms_queue_prefetch",
+  int max_750ms_prefetch = trial_->AppendGroup("max_750ms_queue_prefetch",
                                                kProbabilityPerGroup);
   // Set congestion detection at 2 seconds instead of the 1 second default.
-  int max_2s_prefetch = trial_->AppendGroup("_max_2s_queue_prefetch",
+  int max_2s_prefetch = trial_->AppendGroup("max_2s_queue_prefetch",
                                             kProbabilityPerGroup);
   // Experiment 2:
   // Set max simultaneous resoultions to 2, 4, or 6, and scale the congestion
   // limit proportionally (so we don't impact average probability of asserting
   // congesion very much).
   int max_2_concurrent_prefetch = trial_->AppendGroup(
-        "_max_2 concurrent_prefetch", kProbabilityPerGroup);
+      "max_2 concurrent_prefetch", kProbabilityPerGroup);
   int max_4_concurrent_prefetch = trial_->AppendGroup(
-        "_max_4 concurrent_prefetch", kProbabilityPerGroup);
+      "max_4 concurrent_prefetch", kProbabilityPerGroup);
   int max_6_concurrent_prefetch = trial_->AppendGroup(
-        "_max_6 concurrent_prefetch", kProbabilityPerGroup);
+      "max_6 concurrent_prefetch", kProbabilityPerGroup);
 
-  trial_->AppendGroup("_default_enabled_prefetch",
+  trial_->AppendGroup("default_enabled_prefetch",
       FieldTrial::kAllRemainingProbability);
 
   // We will register the incognito observer regardless of whether prefetching
diff --git a/chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc b/chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
index 9b56611..d7effc9 100644
--- a/chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
+++ b/chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
@@ -28,7 +28,7 @@ void WebSocketExperimentRunner::Start() {
   DCHECK(!runner.get());
 
   scoped_refptr<FieldTrial> trial = new FieldTrial("WebSocketExperiment", 1000);
-  trial->AppendGroup("_active", 5);  // 0.5% in _active group.
+  trial->AppendGroup("active", 5);  // 0.5% in active group.
 
   bool run_experiment = (trial->group() != FieldTrial::kNotParticipating);
 #ifndef NDEBUG