Clean up some code in variations_util.
Shares HashName() implementation between multiple files.
Cleans up some unecessary namespaces.
BUG=none
TEST=existing unit tests
Review URL: https://chromiumcodereview.appspot.com/10920048
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@154989 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/common/metrics/entropy_provider.cc b/chrome/common/metrics/entropy_provider.cc
index ac7c0639..13081d8 100644
--- a/chrome/common/metrics/entropy_provider.cc
+++ b/chrome/common/metrics/entropy_provider.cc
@@ -12,6 +12,7 @@
#include "base/rand_util.h"
#include "base/sha1.h"
#include "base/sys_byteorder.h"
+#include "chrome/common/metrics/metrics_util.h"
namespace metrics {
@@ -43,21 +44,6 @@
return value % range;
}
-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);
-
- uint32 bits;
- COMPILE_ASSERT(sizeof(bits) < sizeof(sha1_hash), need_more_data);
- memcpy(&bits, sha1_hash, sizeof(bits));
-
- return base::ByteSwapToLE32(bits);
-}
-
void PermuteMappingUsingTrialName(const std::string& trial_name,
std::vector<uint16>* mapping) {
for (size_t i = 0; i < mapping->size(); ++i)
diff --git a/chrome/common/metrics/entropy_provider.h b/chrome/common/metrics/entropy_provider.h
index 20584576..7938a5e1 100644
--- a/chrome/common/metrics/entropy_provider.h
+++ b/chrome/common/metrics/entropy_provider.h
@@ -32,11 +32,6 @@
MersenneTwister mersenne_twister_;
};
-// Creates unique identifier for the trial by hashing a name string, whether
-// it's for the field trial or the group name.
-// TODO(asvitkine): Share the implementation with variations_util.cc.
-uint32 HashName(const std::string& name);
-
// Fills |mapping| to create a bijection of values in the range of
// [0, |mapping.size()|), permuted based on |trial_name|.
void PermuteMappingUsingTrialName(const std::string& trial_name,
diff --git a/chrome/common/metrics/entropy_provider_unittest.cc b/chrome/common/metrics/entropy_provider_unittest.cc
index b54a5d50..5b4d56b 100644
--- a/chrome/common/metrics/entropy_provider_unittest.cc
+++ b/chrome/common/metrics/entropy_provider_unittest.cc
@@ -12,6 +12,7 @@
#include "base/rand_util.h"
#include "base/string_number_conversions.h"
#include "chrome/common/metrics/entropy_provider.h"
+#include "chrome/common/metrics/metrics_util.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace metrics {
@@ -308,7 +309,7 @@
const int kMaxAttempts = 1000000;
for (size_t i = 0; i < arraysize(kTestTrialNames); ++i) {
- const uint32 seed = internal::HashName(kTestTrialNames[i]);
+ const uint32 seed = HashName(kTestTrialNames[i]);
internal::SeededRandGenerator rand_generator(seed);
double cumulative_average = 0.0;
diff --git a/chrome/common/metrics/metrics_util.cc b/chrome/common/metrics/metrics_util.cc
new file mode 100644
index 0000000..775501d
--- /dev/null
+++ b/chrome/common/metrics/metrics_util.cc
@@ -0,0 +1,27 @@
+// 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.
+
+#include "chrome/common/metrics/metrics_util.h"
+
+#include "base/sha1.h"
+#include "base/sys_byteorder.h"
+
+namespace metrics {
+
+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);
+
+ uint32 bits;
+ COMPILE_ASSERT(sizeof(bits) < sizeof(sha1_hash), need_more_data);
+ memcpy(&bits, sha1_hash, sizeof(bits));
+
+ return base::ByteSwapToLE32(bits);
+}
+
+} // namespace metrics
diff --git a/chrome/common/metrics/metrics_util.h b/chrome/common/metrics/metrics_util.h
new file mode 100644
index 0000000..306ed09c
--- /dev/null
+++ b/chrome/common/metrics/metrics_util.h
@@ -0,0 +1,20 @@
+// 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_METRICS_UTIL_H_
+#define CHROME_COMMON_METRICS_METRICS_UTIL_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+
+namespace metrics {
+
+// Computes a uint32 hash of a given string based on its SHA1 hash. Suitable for
+// uniquely identifying field trial names and group names.
+uint32 HashName(const std::string& name);
+
+} // namespace metrics
+
+#endif // CHROME_COMMON_METRICS_METRICS_UTIL_H_
diff --git a/chrome/common/metrics/metrics_util_unittest.cc b/chrome/common/metrics/metrics_util_unittest.cc
new file mode 100644
index 0000000..7c21dfd4
--- /dev/null
+++ b/chrome/common/metrics/metrics_util_unittest.cc
@@ -0,0 +1,30 @@
+// 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.
+
+#include "chrome/common/metrics/metrics_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace metrics {
+
+TEST(MetricsUtilTest, HashName) {
+ // Checks that 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, HashName(known_hashes[i].name));
+}
+
+} // namespace metrics
diff --git a/chrome/common/metrics/variations/variations_util.cc b/chrome/common/metrics/variations/variations_util.cc
index 5d09145..fcc5ed3 100644
--- a/chrome/common/metrics/variations/variations_util.cc
+++ b/chrome/common/metrics/variations/variations_util.cc
@@ -14,8 +14,11 @@
#include "base/sys_byteorder.h"
#include "base/utf_string_conversions.h"
#include "chrome/common/child_process_logging.h"
+#include "chrome/common/metrics/metrics_util.h"
#include "chrome/common/metrics/variations/variation_ids.h"
+namespace chrome_variations {
+
namespace {
// The internal singleton accessor for the map, used to keep it thread-safe.
@@ -31,8 +34,8 @@
// Note that this normally only sets the ID for a group the first time, unless
// |force| is set to true, in which case it will always override it.
- void AssociateID(const chrome_variations::SelectedGroupId& group_identifier,
- chrome_variations::VariationID id,
+ void AssociateID(const SelectedGroupId& group_identifier,
+ const VariationID id,
const bool force) {
base::AutoLock scoped_lock(lock_);
if (force ||
@@ -40,54 +43,36 @@
group_to_id_map_[group_identifier] = id;
}
- chrome_variations::VariationID GetID(
- const chrome_variations::SelectedGroupId& group_identifier) {
+ VariationID GetID(const SelectedGroupId& 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())
- return chrome_variations::kEmptyID;
+ return kEmptyID;
return it->second;
}
private:
- typedef std::map<chrome_variations::SelectedGroupId,
- chrome_variations::VariationID,
- chrome_variations::SelectedGroupIdCompare> GroupToIDMap;
+ typedef std::map<SelectedGroupId, VariationID, SelectedGroupIdCompare>
+ GroupToIDMap;
base::Lock lock_;
GroupToIDMap group_to_id_map_;
+
+ DISALLOW_COPY_AND_ASSIGN(GroupMapAccessor);
};
-// 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));
-
- return base::ByteSwapToLE32(bits);
-}
-
-chrome_variations::SelectedGroupId MakeSelectedGroupId(
- const std::string& trial_name,
- const std::string& group_name) {
- chrome_variations::SelectedGroupId id;
- id.name = HashName(trial_name);
- id.group = HashName(group_name);
+SelectedGroupId MakeSelectedGroupId(const std::string& trial_name,
+ const std::string& group_name) {
+ SelectedGroupId id;
+ id.name = metrics::HashName(trial_name);
+ id.group = metrics::HashName(group_name);
return id;
}
// Populates |name_group_ids| based on |selected_groups|.
void GetFieldTrialSelectedGroupIdsForSelectedGroups(
const base::FieldTrial::SelectedGroups& selected_groups,
- std::vector<chrome_variations::SelectedGroupId>* name_group_ids) {
+ std::vector<SelectedGroupId>* name_group_ids) {
DCHECK(name_group_ids->empty());
for (base::FieldTrial::SelectedGroups::const_iterator it =
selected_groups.begin(); it != selected_groups.end(); ++it) {
@@ -97,8 +82,6 @@
} // namespace
-namespace chrome_variations {
-
void GetFieldTrialSelectedGroupIds(
std::vector<SelectedGroupId>* name_group_ids) {
DCHECK(name_group_ids->empty());
@@ -162,21 +145,18 @@
child_process_logging::SetExperimentList(experiment_strings);
}
-} // namespace chrome_variations
-
// Functions below are exposed for testing explicitly behind this namespace.
// They simply wrap existing functions in this file.
namespace testing {
void TestGetFieldTrialSelectedGroupIdsForSelectedGroups(
const base::FieldTrial::SelectedGroups& selected_groups,
- std::vector<chrome_variations::SelectedGroupId>* name_group_ids) {
- ::GetFieldTrialSelectedGroupIdsForSelectedGroups(selected_groups,
- name_group_ids);
-}
-
-uint32 TestHashName(const std::string& name) {
- return ::HashName(name);
+ std::vector<SelectedGroupId>* name_group_ids) {
+ GetFieldTrialSelectedGroupIdsForSelectedGroups(selected_groups,
+ name_group_ids);
}
} // namespace testing
+
+} // namespace chrome_variations
+
diff --git a/chrome/common/metrics/variations/variations_util.h b/chrome/common/metrics/variations/variations_util.h
index 5699b60..df8de02 100644
--- a/chrome/common/metrics/variations/variations_util.h
+++ b/chrome/common/metrics/variations/variations_util.h
@@ -109,18 +109,16 @@
// them to the child process logging module so it can save it for crash dumps.
void SetChildProcessLoggingVariationList();
-} // namespace chrome_variations
-
// Expose some functions for testing. These functions just wrap functionality
// that is implemented above.
namespace testing {
void TestGetFieldTrialSelectedGroupIdsForSelectedGroups(
const base::FieldTrial::SelectedGroups& selected_groups,
- std::vector<chrome_variations::SelectedGroupId>* name_group_ids);
-
-uint32 TestHashName(const std::string& name);
+ std::vector<SelectedGroupId>* name_group_ids);
} // namespace testing
+} // namespace chrome_variations
+
#endif // CHROME_COMMON_METRICS_VARIATIONS_VARIATIONS_UTIL_H_
diff --git a/chrome/common/metrics/variations/variations_util_unittest.cc b/chrome/common/metrics/variations/variations_util_unittest.cc
index 63567f3..352a8a2 100644
--- a/chrome/common/metrics/variations/variations_util_unittest.cc
+++ b/chrome/common/metrics/variations/variations_util_unittest.cc
@@ -11,6 +11,7 @@
#include "base/metrics/field_trial.h"
#include "base/time.h"
#include "base/utf_string_conversions.h"
+#include "chrome/common/metrics/metrics_util.h"
#include "chrome/common/metrics/variations/variations_util.h"
#include "content/public/test/test_browser_thread.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -22,25 +23,25 @@
// Convenience helper to retrieve the chrome_variations::VariationID for a
// FieldTrial. Note that this will do the group assignment in |trial| if not
// already done.
-chrome_variations::VariationID GetIDForTrial(base::FieldTrial* trial) {
+VariationID GetIDForTrial(base::FieldTrial* trial) {
return GetGoogleVariationID(trial->name(), trial->group_name());
}
} // namespace
-class VariationsHelperTest : public ::testing::Test {
+class VariationsUtilTest : public ::testing::Test {
public:
- VariationsHelperTest() : field_trial_list_(NULL) {
+ VariationsUtilTest() : field_trial_list_(NULL) {
// Since the API can only be called on the UI thread, we have to fake that
// we're on it.
ui_thread_.reset(new content::TestBrowserThread(
content::BrowserThread::UI, &message_loop_));
base::Time now = base::Time::NowFromSystemTime();
- base::TimeDelta oneYear = base::TimeDelta::FromDays(365);
+ base::TimeDelta one_year = base::TimeDelta::FromDays(365);
base::Time::Exploded exploded;
- base::Time next_year_time = now + oneYear;
+ base::Time next_year_time = now + one_year;
next_year_time.LocalExplode(&exploded);
next_year_ = exploded.year;
}
@@ -54,28 +55,7 @@
scoped_ptr<content::TestBrowserThread> ui_thread_;
};
-TEST_F(VariationsHelperTest, 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(VariationsHelperTest, GetFieldTrialSelectedGroups) {
+TEST_F(VariationsUtilTest, GetFieldTrialSelectedGroups) {
typedef std::set<SelectedGroupId, SelectedGroupIdCompare> SelectedGroupIdSet;
std::string trial_one("trial one");
std::string group_one("group one");
@@ -95,11 +75,11 @@
// Create our expected groups of IDs.
SelectedGroupIdSet expected_groups;
SelectedGroupId name_group_id;
- name_group_id.name = testing::TestHashName(trial_one);
- name_group_id.group = testing::TestHashName(group_one);
+ name_group_id.name = metrics::HashName(trial_one);
+ name_group_id.group = metrics::HashName(group_one);
expected_groups.insert(name_group_id);
- name_group_id.name = testing::TestHashName(trial_two);
- name_group_id.group = testing::TestHashName(group_two);
+ name_group_id.name = metrics::HashName(trial_two);
+ name_group_id.group = metrics::HashName(group_two);
expected_groups.insert(name_group_id);
std::vector<SelectedGroupId> selected_group_ids;
@@ -117,7 +97,7 @@
// Test that if the trial is immediately disabled, GetGoogleVariationID just
// returns the empty ID.
-TEST_F(VariationsHelperTest, DisableImmediately) {
+TEST_F(VariationsUtilTest, DisableImmediately) {
int default_group_number = -1;
scoped_refptr<base::FieldTrial> trial(
base::FieldTrialList::FactoryGetFieldTrial("trial", 100, "default",
@@ -126,13 +106,13 @@
trial->Disable();
ASSERT_EQ(default_group_number, trial->group());
- ASSERT_EQ(chrome_variations::kEmptyID, GetIDForTrial(trial.get()));
+ ASSERT_EQ(kEmptyID, GetIDForTrial(trial.get()));
}
// Test that successfully associating the FieldTrial with some ID, and then
// disabling the FieldTrial actually makes GetGoogleVariationID correctly
// return the empty ID.
-TEST_F(VariationsHelperTest, DisableAfterInitialization) {
+TEST_F(VariationsUtilTest, DisableAfterInitialization) {
const std::string default_name = "default";
const std::string non_default_name = "non_default";
@@ -140,19 +120,17 @@
base::FieldTrialList::FactoryGetFieldTrial("trial", 100, default_name,
next_year_, 12, 12, NULL));
trial->AppendGroup(non_default_name, 100);
- AssociateGoogleVariationID(
- trial->name(), default_name, chrome_variations::kTestValueA);
- AssociateGoogleVariationID(
- trial->name(), non_default_name, chrome_variations::kTestValueB);
+ AssociateGoogleVariationID(trial->name(), default_name, kTestValueA);
+ AssociateGoogleVariationID(trial->name(), non_default_name, kTestValueB);
ASSERT_EQ(non_default_name, trial->group_name());
- ASSERT_EQ(chrome_variations::kTestValueB, GetIDForTrial(trial.get()));
+ ASSERT_EQ(kTestValueB, GetIDForTrial(trial.get()));
trial->Disable();
ASSERT_EQ(default_name, trial->group_name());
- ASSERT_EQ(chrome_variations::kTestValueA, GetIDForTrial(trial.get()));
+ ASSERT_EQ(kTestValueA, GetIDForTrial(trial.get()));
}
// Test various successful association cases.
-TEST_F(VariationsHelperTest, AssociateGoogleVariationID) {
+TEST_F(VariationsUtilTest, AssociateGoogleVariationID) {
const std::string default_name1 = "default1";
scoped_refptr<base::FieldTrial> trial_true(
base::FieldTrialList::FactoryGetFieldTrial("d1", 10, default_name1,
@@ -161,66 +139,56 @@
int winner_group = trial_true->AppendGroup(winner, 10);
// Set GoogleVariationIDs so we can verify that they were chosen correctly.
- AssociateGoogleVariationID(
- trial_true->name(), default_name1, chrome_variations::kTestValueA);
- AssociateGoogleVariationID(
- trial_true->name(), winner, chrome_variations::kTestValueB);
+ AssociateGoogleVariationID(trial_true->name(), default_name1, kTestValueA);
+ AssociateGoogleVariationID(trial_true->name(), winner, kTestValueB);
EXPECT_EQ(winner_group, trial_true->group());
EXPECT_EQ(winner, trial_true->group_name());
- EXPECT_EQ(chrome_variations::kTestValueB, GetIDForTrial(trial_true.get()));
+ EXPECT_EQ(kTestValueB, GetIDForTrial(trial_true.get()));
const std::string default_name2 = "default2";
scoped_refptr<base::FieldTrial> trial_false(
base::FieldTrialList::FactoryGetFieldTrial("d2", 10, default_name2,
next_year_, 12, 31, NULL));
const std::string loser = "ALoser";
- int loser_group = trial_false->AppendGroup(loser, 0);
+ const int loser_group = trial_false->AppendGroup(loser, 0);
- AssociateGoogleVariationID(
- trial_false->name(), default_name2, chrome_variations::kTestValueA);
- AssociateGoogleVariationID(
- trial_false->name(), loser, chrome_variations::kTestValueB);
+ AssociateGoogleVariationID(trial_false->name(), default_name2, kTestValueA);
+ AssociateGoogleVariationID(trial_false->name(), loser, kTestValueB);
EXPECT_NE(loser_group, trial_false->group());
- EXPECT_EQ(chrome_variations::kTestValueA, GetIDForTrial(trial_false.get()));
+ EXPECT_EQ(kTestValueA, GetIDForTrial(trial_false.get()));
}
// Test that not associating a FieldTrial with any IDs ensure that the empty ID
// will be returned.
-TEST_F(VariationsHelperTest, NoAssociation) {
+TEST_F(VariationsUtilTest, NoAssociation) {
const std::string default_name = "default";
scoped_refptr<base::FieldTrial> no_id_trial(
base::FieldTrialList::FactoryGetFieldTrial("d3", 10, default_name,
next_year_, 12, 31, NULL));
const std::string winner = "TheWinner";
- int winner_group = no_id_trial->AppendGroup(winner, 10);
+ const int winner_group = no_id_trial->AppendGroup(winner, 10);
// Ensure that despite the fact that a normal winner is elected, it does not
- // have a valid chrome_variations::VariationID associated with it.
+ // have a valid VariationID associated with it.
EXPECT_EQ(winner_group, no_id_trial->group());
EXPECT_EQ(winner, no_id_trial->group_name());
- EXPECT_EQ(chrome_variations::kEmptyID, GetIDForTrial(no_id_trial.get()));
+ EXPECT_EQ(kEmptyID, GetIDForTrial(no_id_trial.get()));
}
// Ensure that the AssociateGoogleVariationIDForce works as expected.
-TEST_F(VariationsHelperTest, ForceAssociation) {
- EXPECT_EQ(chrome_variations::kEmptyID,
- GetGoogleVariationID("trial", "group"));
- AssociateGoogleVariationID("trial", "group",
- chrome_variations::kTestValueA);
- EXPECT_EQ(chrome_variations::kTestValueA,
- GetGoogleVariationID("trial", "group"));
- AssociateGoogleVariationID("trial", "group", chrome_variations::kTestValueB);
- EXPECT_EQ(chrome_variations::kTestValueA,
- GetGoogleVariationID("trial", "group"));
- AssociateGoogleVariationIDForce("trial", "group",
- chrome_variations::kTestValueB);
- EXPECT_EQ(chrome_variations::kTestValueB,
- GetGoogleVariationID("trial", "group"));
+TEST_F(VariationsUtilTest, ForceAssociation) {
+ EXPECT_EQ(kEmptyID, GetGoogleVariationID("trial", "group"));
+ AssociateGoogleVariationID("trial", "group", kTestValueA);
+ EXPECT_EQ(kTestValueA, GetGoogleVariationID("trial", "group"));
+ AssociateGoogleVariationID("trial", "group", kTestValueB);
+ EXPECT_EQ(kTestValueA, GetGoogleVariationID("trial", "group"));
+ AssociateGoogleVariationIDForce("trial", "group", kTestValueB);
+ EXPECT_EQ(kTestValueB, GetGoogleVariationID("trial", "group"));
}
-TEST_F(VariationsHelperTest, GenerateExperimentChunks) {
+TEST_F(VariationsUtilTest, GenerateExperimentChunks) {
const char* kExperimentStrings[] = {
"1d3048f1-9de009d0",
"cd73da34-cf196cb",
