Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper_unittest.cc

Index: chrome/common/metrics/experiments_helper_unittest.cc
diff --git a/chrome/common/metrics/experiments_helper_unittest.cc b/chrome/common/metrics/experiments_helper_unittest.cc
index 546cce86c3ebe5abd84c0a97743933e850876b08..7a93896c75de6229386471c0ff19f9f751769ac8 100644
--- a/chrome/common/metrics/experiments_helper_unittest.cc
+++ b/chrome/common/metrics/experiments_helper_unittest.cc
@@ -12,14 +12,15 @@
 #include "content/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#include <set>
+
 namespace {
 
 // Convenience helper to retrieve the GoogleExperimentID for a FieldTrial. Note
 // that this will do the group assignment in |trial| if not already done.
 experiments_helper::GoogleExperimentID GetIDForTrial(base::FieldTrial* trial) {
   return experiments_helper::GetGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(),
-                                        trial->group_name()));
+      testing::TestMakeNameGroupId(trial->name(), trial->group_name()));
 }
 
 }  // namespace
@@ -49,6 +50,63 @@ class ExperimentsHelperTest : public ::testing::Test {
   scoped_ptr<content::TestBrowserThread> ui_thread_;
 };
 
+TEST_F(ExperimentsHelperTest, HashName) {
+  // Make sure hashing is stable on all platforms.
+  struct {
+    const char* name;
+    uint32 hash_value;
+  } known_hashes[] = {
+    {"a", 937752454u},
+    {"1", 723085877u},
+    {"Trial Name", 2713117220u},
+    {"Group Name", 3201815843u},
+    {"My Favorite Experiment", 3722155194u},
+    {"My Awesome Group Name", 4109503236u},
+    {"abcdefghijklmonpqrstuvwxyz", 787728696u},
+    {"0123456789ABCDEF", 348858318U}
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(known_hashes); ++i) {
+    EXPECT_EQ(known_hashes[i].hash_value,
+              testing::TestHashName(known_hashes[i].name));
+  }
+}
+
+TEST_F(ExperimentsHelperTest, GetFieldTrialSelectedGroups) {
+  std::string trial_one("trial one");
+  std::string group_one("group one");
+  std::string trial_two("trial two");
+  std::string group_two("group two");
+
+  base::FieldTrial::SelectedGroups selected_groups;
+  base::FieldTrial::SelectedGroup selected_group;
+  selected_group.trial = trial_one;
+  selected_group.group = group_one;
+  selected_groups.push_back(selected_group);
+
+  selected_group.trial = trial_two;
+  selected_group.group = group_two;
+  selected_groups.push_back(selected_group);
+
+  // Create our expected groups of IDs.
+  std::set<experiments_helper::NameGroupId,
+      experiments_helper::NameGroupIdCompare> expected_groups;
+  expected_groups.insert(testing::TestMakeNameGroupId(trial_one, group_one));
+  expected_groups.insert(testing::TestMakeNameGroupId(trial_two, group_two));
+
+  std::vector<experiments_helper::NameGroupId> name_group_ids;
+  testing::TestGetFieldTrialNameGroupIdsForSelectedGroups(selected_groups,
+                                                         &name_group_ids);
+  EXPECT_EQ(2U, name_group_ids.size());
+  for (size_t i = 0; i < name_group_ids.size(); ++i) {
+    std::set<experiments_helper::NameGroupId,
+        experiments_helper::NameGroupIdCompare>::iterator it =
+            expected_groups.find(name_group_ids[i]);
+    EXPECT_NE(it, expected_groups.end());
+    expected_groups.erase(it);

[MAD, 2012/04/26 19:40:13]

I think it's dangerous for some container classes to erase an entity with an
iterator and then increment this iterator... It doesn't always work, you may
skip elements...

So that might be another good incentive to use the trick I propose in the
previous comment... Have you walked this code in the debugger to make sure you
go through the loop twice?

[SteveT, 2012/04/26 21:06:03]

Oops, maybe it's not clear, but there are actually two containers here... a
vector of produced NameGroupIds, which we iterate over, and a set of expected
NameGroupIds, which we remove from to verify that we see all expected
NameGroupIds (the set is expected to be empty if we successfully see every
NameGroupId, which we check below).

So basically the iterator we erase isn't the same one we're looping over.

Does this make sense, or is this pattern a bit too convoluted and you'd rather I
use your trick?

[MAD, 2012/04/27 17:33:22]

OK, that's fine... The trick I propose would prevent the need to expose the
compare function publicly, but that's fine... I think it might be more obvious
with a C style array, but that might just be me... So I'm OK with your
solution... Except...

To resolve the potential ambiguity, you could use a more meaningful name for the
set iterator... :-)

Thanks!

[SteveT, 2012/04/27 17:59:33]

Good point. I've renamed it to expected_group, which is more clear. Also noticed
that I have to renaming name_group_ids here. Thanks!

+  }
+  EXPECT_EQ(0U, expected_groups.size());
+}
+
 // Test that if the trial is immediately disabled, GetGoogleExperimentID just
 // returns the empty ID.
 TEST_F(ExperimentsHelperTest, DisableImmediately) {
@@ -76,9 +134,9 @@ TEST_F(ExperimentsHelperTest, DisableAfterInitialization) {
                                                  next_year_, 12, 12, NULL));
   trial->AppendGroup(non_default_name, 100);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(), default_name), 123);
+      testing::TestMakeNameGroupId(trial->name(), default_name), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(), non_default_name), 456);
+      testing::TestMakeNameGroupId(trial->name(), non_default_name), 456);
   ASSERT_EQ(non_default_name, trial->group_name());
   ASSERT_EQ(456U, GetIDForTrial(trial.get()));
   trial->Disable();
@@ -97,11 +155,9 @@ TEST_F(ExperimentsHelperTest, AssociateGoogleExperimentID) {
 
   // Set GoogleExperimentIDs so we can verify that they were chosen correctly.
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_true->name(), default_name1),
-      123);
+      testing::TestMakeNameGroupId(trial_true->name(), default_name1), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_true->name(), winner),
-      456);
+      testing::TestMakeNameGroupId(trial_true->name(), winner), 456);
 
   EXPECT_EQ(winner_group, trial_true->group());
   EXPECT_EQ(winner, trial_true->group_name());
@@ -115,11 +171,9 @@ TEST_F(ExperimentsHelperTest, AssociateGoogleExperimentID) {
   int loser_group = trial_false->AppendGroup(loser, 0);
 
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_false->name(), default_name2),
-      123);
+      testing::TestMakeNameGroupId(trial_false->name(), default_name2), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_false->name(), loser),
-      456);
+      testing::TestMakeNameGroupId(trial_false->name(), loser), 456);
 
   EXPECT_NE(loser_group, trial_false->group());
   EXPECT_EQ(123U, GetIDForTrial(trial_false.get()));