Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper.cc

Index: chrome/common/metrics/experiments_helper.cc
diff --git a/chrome/common/metrics/experiments_helper.cc b/chrome/common/metrics/experiments_helper.cc
index 11c7c1389fca180304be1513a6498446e06d441e..e734f944b66a80125cc4c2c76d75d789dcefc353 100644
--- a/chrome/common/metrics/experiments_helper.cc
+++ b/chrome/common/metrics/experiments_helper.cc
@@ -7,18 +7,22 @@
 #include <vector>
 
 #include "base/memory/singleton.h"
+#include "base/sha1.h"
 #include "base/string16.h"
 #include "base/stringprintf.h"
+#include "base/sys_byteorder.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/child_process_logging.h"
 
 namespace {
 
+const uint32 kReservedHashValue = 0;
+
 // We need to pass a Compare class to the std::map template since NameGroupId
 // is a user-defined type.
 struct NameGroupIdCompare {
-  bool operator() (const base::FieldTrial::NameGroupId& lhs,
-                   const base::FieldTrial::NameGroupId& rhs) const {
+  bool operator() (const experiments_helper::NameGroupId& lhs,
+                   const experiments_helper::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.
@@ -38,7 +42,7 @@ class GroupMapAccessor {
   GroupMapAccessor() {}
   ~GroupMapAccessor() {}
 
-  void AssociateID(const base::FieldTrial::NameGroupId& group_identifier,
+  void AssociateID(const experiments_helper::NameGroupId& group_identifier,
                    experiments_helper::GoogleExperimentID id) {
     base::AutoLock scoped_lock(lock_);
     DCHECK(group_to_id_map_.find(group_identifier) == group_to_id_map_.end()) <<
@@ -47,7 +51,7 @@ class GroupMapAccessor {
   }
 
   experiments_helper::GoogleExperimentID GetID(
-      const base::FieldTrial::NameGroupId& group_identifier) {
+      const experiments_helper::NameGroupId& group_identifier) {
     base::AutoLock scoped_lock(lock_);
     GroupToIDMap::const_iterator it = group_to_id_map_.find(group_identifier);
     if (it == group_to_id_map_.end())
@@ -56,33 +60,85 @@ class GroupMapAccessor {
   }
 
  private:
-  typedef std::map<base::FieldTrial::NameGroupId,
+  typedef std::map<experiments_helper::NameGroupId,
       experiments_helper::GoogleExperimentID, NameGroupIdCompare> GroupToIDMap;
 
   base::Lock lock_;
   GroupToIDMap group_to_id_map_;
 };
 
+// Creates unique identifier for the trial by hashing a name string, whether
+// it's for the field trial or the group name.
+uint32 HashName(const std::string& name) {
+  // SHA-1 is designed to produce a uniformly random spread in its output space,
+  // even for nearly-identical inputs.
+  unsigned char sha1_hash[base::kSHA1Length];
+  base::SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()),
+                      name.size(),
+                      sha1_hash);
+
+  COMPILE_ASSERT(sizeof(uint32) < sizeof(sha1_hash), need_more_data);
+  uint32 bits;
+  memcpy(&bits, sha1_hash, sizeof(bits));
+
+  // We only DCHECK, since this should not happen because the registration
+  // of the experiment on the server should have already warn the developer.
+  // If this ever happen, we'll ignore this experiment/group when reporting.
+  DCHECK(bits != kReservedHashValue);

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

Actually, I don't think we need this anymore. This was to initialize the member
until it got set... Since we don't have a member to initialize anymore, this is
OK...

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

Ah okay, I was wondering about that. I've removed this check and the constant
(since it's the only user).

+  return base::ByteSwapToLE32(bits);
+}
+
 }  // namespace
 
 namespace experiments_helper {
 
 const GoogleExperimentID kEmptyGoogleExperimentID = 0;
 
+void GetFieldTrialNameGroupIds(std::vector<NameGroupId>* name_group_ids) {
+  DCHECK(name_group_ids->empty());
+  // A note on thread safety: Since GetFieldTrialSelectedGroups is thread
+  // safe, and we operate on a separate list of that data, this function is
+  // technically thread safe as well, with respect to the FieldTriaList data.
+  SelectedGroups selected_groups;
+  base::FieldTrialList::GetFieldTrialSelectedGroups(&selected_groups);
+  GetFieldTrialNameGroupIdsForSelectedGroups(selected_groups, name_group_ids);
+}
+
+void GetFieldTrialNameGroupIdsForSelectedGroups(

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

Maybe add a comment about the separation from previous function for testing
purposes...

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

Cool. I have a comment around the testing namespace for this.

+    const SelectedGroups& selected_groups,
+    std::vector<NameGroupId>* name_group_ids) {
+  DCHECK(name_group_ids->empty());
+  for (SelectedGroups::const_iterator it = selected_groups.begin();
+       it != selected_groups.end(); ++it)

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

We use {} when the condition/loop has more than one line...
You could also put the declaration of the iterator on a separate line before the
for loop to avoid that.

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

I do recall pkasting telling me that they are only needed if the loop contents
is > 1 line, and it doesn't matter if the condition has > 1 line... But I'll add
the braces here since I think it's a bit neater.

+    name_group_ids->push_back(MakeNameGroupId(it->trial, it->group));
+}
+
+NameGroupId MakeNameGroupId(const std::string& trial_name,
+                            const std::string& group_name) {
+  NameGroupId id;
+  id.name = HashName(trial_name);
+  id.group = HashName(group_name);
+  return id;
+}
+
+uint32 TestingHashName(const std::string& name) {
+  return HashName(name);
+}
+
 void AssociateGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier,
+    const NameGroupId& group_identifier,
     GoogleExperimentID id) {
   GroupMapAccessor::GetInstance()->AssociateID(group_identifier, id);
 }
 
 GoogleExperimentID GetGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier) {
+    const NameGroupId& group_identifier) {
   return GroupMapAccessor::GetInstance()->GetID(group_identifier);
 }
 
 void SetChildProcessLoggingExperimentList() {
-  std::vector<base::FieldTrial::NameGroupId> name_group_ids;
-  base::FieldTrialList::GetFieldTrialNameGroupIds(&name_group_ids);
+  std::vector<NameGroupId> name_group_ids;
+  GetFieldTrialNameGroupIds(&name_group_ids);
   std::vector<string16> experiment_strings(name_group_ids.size());
   for (size_t i = 0; i < name_group_ids.size(); ++i) {
     // Wish there was a StringPrintf for string16... :-(