Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper.h

Index: chrome/common/metrics/experiments_helper.h
diff --git a/chrome/common/metrics/experiments_helper.h b/chrome/common/metrics/experiments_helper.h
index 102e6351c75795bfff3d624294d5b8a89f06f2d9..60c8850e3137eb562e5fed06c6557f36739539d2 100644
--- a/chrome/common/metrics/experiments_helper.h
+++ b/chrome/common/metrics/experiments_helper.h
@@ -28,17 +28,14 @@
 // const int kLowMemGroup = trial->AppendGroup("LowMem", 20);
 // // All groups are now created. We want to associate GoogleExperimentIDs with
 // // them, so do that now.
-// AssociateGoogleExperimentID(
-//     FieldTrial::MakeNameGroupId("trial", "default"), 123);
-// AssociateGoogleExperimentID(
-//     FieldTrial::MakeNameGroupId("trial", "HighMem"), 456);
-// AssociateGoogleExperimentID(
-//     FieldTrial::MakeNameGroupId("trial", "LowMem"), 789);
+// AssociateGoogleExperimentID(MakeNameGroupId("trial", "default"), 123);
+// AssociateGoogleExperimentID(MakeNameGroupId("trial", "HighMem"), 456);
+// AssociateGoogleExperimentID(MakeNameGroupId("trial", "LowMem"), 789);
 //
 // // Elsewhere, we are interested in retrieving the GoogleExperimentID
 // // assocaited with |trial|.
 // GoogleExperimentID id = GetGoogleExperimentID(
-//     FieldTrial::MakeNameGroupId(trial->name(), trial->group_name()));
+//     MakeNameGroupId(trial->name(), trial->group_name()));
 // // Do stuff with |id|...
 //
 // The AssociateGoogleExperimentID and GetGoogleExperimentID API methods are
@@ -48,27 +45,52 @@ namespace experiments_helper {
 // An ID used by Google servers to identify a local browser experiment.
 typedef uint32 GoogleExperimentID;
 
+typedef std::vector<base::FieldTrial::SelectedGroup> SelectedGroups;

[MAD, 2012/04/25 20:03:22]

Maybe we could put this typedef within the FieldTrialList class?

[SteveT, 2012/04/26 19:03:31]

Makes more sense. Done.

+
+// The Unique ID of a trial, where the name and group identifiers are
+// hashes of the trial and group name strings.
+struct NameGroupId {
+  uint32 name;
+  uint32 group;
+};
+
 // Used to represent no associated Google experiment ID. Calls to the
 // GetGoogleExperimentID API below will return this empty value for FieldTrial
 // groups uninterested in associating themselves with Google experiments, or
 // those that have not yet been seen yet.
 extern const GoogleExperimentID kEmptyGoogleExperimentID;
 
+// Returns an array of Unique IDs for each Field Trial that has a chosen
+// group. Field Trials for which a group has not been chosen yet are NOT
+// returned in this list. In the real world, clients should use
+// GetFieldTrialNameGroupIds. We expose
+// GetFieldTrialNameGroupIdsForSelectedGroups for testing, which creates the
+// NameGroupIds based on |selected_groups|.
+void GetFieldTrialNameGroupIds(std::vector<NameGroupId>* name_group_ids);
+void GetFieldTrialNameGroupIdsForSelectedGroups(

[MAD, 2012/04/25 20:03:22]

You could also put this one in an unnamed namespace (as HashName) and expose a
Test method appropriately prefixed...

[SteveT, 2012/04/26 19:03:31]

Done.

+    const SelectedGroups& selected_groups,
+    std::vector<NameGroupId>* name_group_ids);
+
+// Helper function to create a NameGroupId from |trial_name| and |group_name|.
+NameGroupId MakeNameGroupId(const std::string& trial_name,

[MAD, 2012/04/25 20:03:22]

Will this be needed other than by tests?

I'm thinking that we could also only expose a testing::TestXXX method for this,
and maybe we wouldn't need to expose HashName since we can test the hashing by
calling this method instead... What Do You Think?

[SteveT, 2012/04/26 19:03:31]

Technically you need it to get GXIDs for individual FieldTrials, but since only
GetFieldTrialNameGroupIds uses it right now, we can technically hide it and
expose a testing version.

I'm not a fan of using this inside of HashName for tests that require a single
Hash for a verification. Seems hacky.

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

I think it's fine to test HashName only via this method, we don't really need to
test a single name hash, if it works for two, it sure works for one... But I'm
OK with leaving it like this too if you insist... :-)

+                            const std::string& group_name);
+
+// A wrapper used to expose HashName for testing only.
+uint32 TestingHashName(const std::string& name);

[MAD, 2012/04/25 20:03:22]

To make it every clearer, you could put it in the testing namespace, outside of
the experiments_helper namespace.

[SteveT, 2012/04/26 19:03:31]

Done.

+
 // Set the GoogleExperimentID associated with a FieldTrial group. The group is
 // denoted by |group_identifier|, which can be created by passing the
 // FieldTrial's trial and group names to base::FieldTrial::MakeNameGroupId.
 // This does not need to be called for FieldTrials uninterested in Google
 // experiments.
-void AssociateGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier,
-    GoogleExperimentID id);
+void AssociateGoogleExperimentID(const NameGroupId& group_identifier,
+                                 GoogleExperimentID id);
 
 // Retrieve the GoogleExperimentID associated with a FieldTrial group. The group
 // is denoted by |group_identifier| (see comment above). This can be nicely
 // combined with FieldTrial::GetFieldTrialNameGroupIds to enumerate the
 // GoogleExperimentIDs for all active FieldTrial groups.
-GoogleExperimentID GetGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier);
+GoogleExperimentID GetGoogleExperimentID(const NameGroupId& group_identifier);
 
 // Get the current set of chosen FieldTrial groups (aka experiments) and send
 // them to the child process logging module so it can save it for crash dumps.