Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_COMMON_METRICS_EXPERIMENTS_HELPER_H_
 #define CHROME_COMMON_METRICS_EXPERIMENTS_HELPER_H_
 #pragma once
 
 #include "base/metrics/field_trial.h"
 
@@ skipping 10 matching lines @@
 //
 // Typical usage looks like this:
 //
 // // Set up your trial and groups as usual.
 // FieldTrial* trial = FieldTrialList::FactoryGetFieldTrial(
 //     "trial", 1000, "default", 2012, 12, 31, NULL);
 // const int kHighMemGroup = trial->AppendGroup("HighMem", 20);
 // 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("trial", "default", 123);
+// AssociateGoogleExperimentID("trial", "HighMem", 456);
+// AssociateGoogleExperimentID("trial", "LowMem", 789);
 //
 // // Elsewhere, we are interested in retrieving the GoogleExperimentID
 // // assocaited with |trial|.
-// GoogleExperimentID id = GetGoogleExperimentID(
-//     FieldTrial::MakeNameGroupId(trial->name(), trial->group_name()));
+// GoogleExperimentID id = GetGoogleExperimentID(trial->name(),
+//                                               trial->group_name());
 // // Do stuff with |id|...
 //
 // The AssociateGoogleExperimentID and GetGoogleExperimentID API methods are
 // thread safe.
 namespace experiments_helper {
 
 // An ID used by Google servers to identify a local browser experiment.
 typedef uint32 GoogleExperimentID;
 
+// The Unique ID of a trial, where the name and group identifiers are
+// hashes of the trial and group name strings.
+struct NameGroupId {

[jar (doing other things), 2012/04/26 22:24:22]

nit: I found this name hard to guess about.  As it is, SelectedGroup was a
little hard to guess about (it doesn't sound like a "pair" of anything), and now
this is the same data, just hashed, but the struct names of the types are
completely unrelated :-/.

Perhaps SelectedGroup might be GroupInTrial
Then NameGroupId might be HashedGroupInTrial

That is a bit long... but I think the names should be made similar, and easier
to relate to one and other.

Another plausible set of names might be:
SelectedGroup
SelectedGroupHashes

Or maybe even:
SelectedGroup
SelectedGroupIds

Or perhaps:
TrialGroupPair
TrialGroupPairIds

[SteveT, 2012/04/26 23:42:47]

I'll take you up on SelectedGroupIds. "Hashes" is more "accurate", bit we do
see/use the hash-pairs as an identifier (when transmitted to our servers).

+  uint32 name;
+  uint32 group;
+};
+
+// We need to supply a Compare class for templates since NameGroupId is a
+// user-defined type.
+struct NameGroupIdCompare {
+  bool operator() (const NameGroupId& lhs, const NameGroupId& rhs) const {
+    // The group and name fields are just SHA-1 Hashes, so we just need to treat
+    // them as IDs and do a less-than comparison. We test group first, since
+    // name is more likely to collide.
+    return lhs.group == rhs.group ? lhs.name < rhs.name :
+        lhs.group < rhs.group;

[jar (doing other things), 2012/04/26 22:24:22]

nit: (You can ignore... as some folks are in love with a ternary operator):  It
is IMO much easier to read as an if statement, where I don't have to hold lots
of context in my head as I read:

if (lsh.group != rhs.group)
  return lhs.group < rhs.group
return lhs.name < rhs.name;

IF you really really want to write it with the ternary operator, it is probably
easier to read when formatted:

return lhs.group == rhs.group ? lhs.name < rhs.name
                              : lhs.group < rhs.group;

or:

return lhs.group != rhs.group ? lhs.group < rhs.group
                              : lhs.name < rhs.name;

...but all of this harks back to my life as an APL programmer.... and I'd rather
see simple easy-to-scan statements ;-).

[SteveT, 2012/04/26 23:42:47]

I personally find ternaries pretty easy to read*, but I do agree that reading
your suggested code requires slightly less context. Done.

* I generally parse ternary statements in my head as:

[is this true] ? [if so, use this]
               : [otherwise, this]

I find the "?" puts the inflection in just the right place so that it makes
sense in English. But maybe most folks don't read them that way.

+  }
+};
+
 // 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

[jar (doing other things), 2012/04/26 22:24:22]

nit: "Returns an array..."  ---> "Fills in vector |name_group_ids| ..."

[You'll need some English fixin', but "returns an array" sounds like it
"allocates and returns an array" :-/.

[SteveT, 2012/04/26 23:42:47]

Done.

+// 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.
+void GetFieldTrialNameGroupIds(std::vector<NameGroupId>* name_group_ids);
+
 // 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);
+// denoted by |trial_name| and |group_name|. This does not need to be called for
+// FieldTrials uninterested in Google experiments.

[jar (doing other things), 2012/04/26 22:24:22]

I couldn't parse the comment, and understand what it does.
For instance, I wasn't sure what:
"Set the GoogleExperimentID" was meant to do.
I wasn't clear about the personification of "FieldTrials" as being "interested
in Google experiments."

It is probably not appropriate to comment about when a function "does not need
to be called," and better to say when it must be called.  Even then, this might
be a passive voice, and it is not clear who should, or when this should be
called.

[SteveT, 2012/04/26 23:42:47]

Okay, I hope this is a little bit better. We avoid mentioning when it is not
needed.

[jar (doing other things), 2012/04/27 19:05:44]

Perfect! Thanks!

On 2012/04/26 23:42:47, SteveT wrote:

+void AssociateGoogleExperimentID(const std::string& trial_name,
+                                 const std::string& group_name,
+                                 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);
+// is denoted by |trial_name| and |group_name|. This can be nicely combined with
+// FieldTrial::GetFieldTrialNameGroupIds to enumerate the GoogleExperimentIDs
+// for all active FieldTrial groups.
+GoogleExperimentID GetGoogleExperimentID(const std::string& trial_name,
+                                         const std::string& group_name);
 
 // 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.
 void SetChildProcessLoggingExperimentList();
 
 }  // namespace experiments_helper
 
+// Expose some functions for testing. These functions just wrap functionality
+// that is implemented above.
+namespace testing {
+
+void TestGetFieldTrialNameGroupIdsForSelectedGroups(
+    const base::FieldTrial::SelectedGroups& selected_groups,
+    std::vector<experiments_helper::NameGroupId>* name_group_ids);
+
+uint32 TestHashName(const std::string& name);
+
+}
+
 #endif  // CHROME_COMMON_METRICS_EXPERIMENTS_HELPER_H_