Added of new UMA signals in order to be able to discover early if the "save password" feature gets …

chrome/browser/password_manager/password_manager_metrics_util.cc

Index: chrome/browser/password_manager/password_manager_metrics_util.cc
diff --git a/chrome/browser/password_manager/password_manager_metrics_util.cc b/chrome/browser/password_manager/password_manager_metrics_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..32e12e065974ed7a983ae85db79fb595a74e51fb
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_metrics_util.cc
@@ -0,0 +1,118 @@
+// Copyright 2013 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/browser/password_manager/password_manager_metrics_util.h"
+
+#include "base/basictypes.h"
+#include "base/metrics/histogram.h"
+#include "base/process/process_handle.h"

[Ilya Sherman, 2013/09/12 04:45:45]

nit: No longer needed?

[jdomingos, 2013/09/12 12:01:01]

Done.

+#include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"

[Ilya Sherman, 2013/09/12 04:45:45]

nit: No longer needed?

+#include "url/gurl.h"
+
+namespace password_manager_metrics_util {
+
+namespace {
+
+// The number of domain group.

[Ilya Sherman, 2013/09/12 04:45:45]

nit: "group" -> "groups"

[jdomingos, 2013/09/12 12:01:01]

Done.

+const size_t kGroupNumber = 20u;

[Ilya Sherman, 2013/09/12 04:45:45]

nit: "kNumGroups"

[jdomingos, 2013/09/12 12:01:01]

Done.

+
+}  // namespace
+
+// Contains a monitored domain name and all the group IDs which contain
+// domain name.
+struct DomainGroupsPair {
+  const char* const domain_name;
+  const int group_ids[kGroupsPerDomain];
+};
+
+unsigned int g_random_group_id = kGroupsPerDomain;

[Ilya Sherman, 2013/09/12 04:45:45]

Please always prefer size_t or one of the uint* types to unsigned int.

[Ilya Sherman, 2013/09/12 04:45:45]

This should be declared in the anonymous namespace.

[jdomingos, 2013/09/12 12:01:01]

Done.

[jdomingos, 2013/09/12 12:01:01]

Done.

+
+// Generate an id in range [0, |kGroupsPerDomain|).
+// If the |random_group_id| is equal to its initial value (|kGroupsPerDomain|),
+// the value of |random_id| is not changed.
+void GenerateRandomId() {
+  // For each session, the user uses only one group for each website.
+  if (g_random_group_id == kGroupsPerDomain)
+    g_random_group_id = base::RandGenerator(kGroupsPerDomain);
+}
+
+void SetRandomIdForTesting(unsigned int value) {
+  g_random_group_id = value;
+}
+
+std::string AppendGroup(unsigned int group_id) {
+  return "group_" + base::IntToString(group_id);

[Ilya Sherman, 2013/09/12 04:45:45]

Seems like this ought to return an empty string if the group_id corresponds to
an unmonitored domain.

[jdomingos, 2013/09/12 12:01:01]

Done.

+}

[Ilya Sherman, 2013/09/12 04:45:45]

Please make sure that the order of the functions is consistent between the
implementation file and the header file.

[jdomingos, 2013/09/12 12:01:01]

Done.

+
+unsigned int MonitoredDomainGroupId(const std::string& url_host) {
+  // This array contains each monitored websites together with all ids of

[Ilya Sherman, 2013/09/12 04:45:45]

nit: "websites" -> "website"

[jdomingos, 2013/09/12 12:01:01]

Done.

+  // groups which contain the website. Each website appears |kGroupsPerDomain|
+  // times and every |group_ids| is different. For more information about the

[Ilya Sherman, 2013/09/12 04:45:45]

nit: Possible rephrasing of "Each website appears |kGroupsPerDomain| times and
every |group_ids| is different.": "Each website appears in |kGroupsPerDomain|
groups, and each group includes an equal number of websites, so that no two
websites have the same set of groups that they belong to."

[jdomingos, 2013/09/12 12:01:01]

Done.

+  // the algorithm used see http://goo.gl/vUuFd5.
+  static const DomainGroupsPair kDomainMapping[] = {
+      {"google.com", {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}},
+      {"yahoo.com", {1, 2, 3, 4, 5, 11, 12, 13, 14, 15}},
+      {"baidu.com", {1, 2, 3, 4, 6, 7, 11, 12, 16, 17}},
+      {"wikipedia.org", {1, 2, 3, 4, 5, 6, 11, 12, 16, 18}},
+      {"linkedin.com", {1, 6, 8, 11, 13, 14, 15, 16, 17, 19}},
+      {"twitter.com", {5, 6, 7, 8, 9, 11, 13, 17, 19, 20}},
+      {"live.com", {7, 8, 9, 10, 13, 14, 16, 17, 18, 20}},
+      {"amazon.com", {2, 5, 9, 10, 12, 14, 15, 18, 19, 20}},
+      {"ebay.com", {3, 7, 9, 10, 14, 15, 17, 18, 19, 20}},
+      {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}},
+  };
+  const size_t kDomainMappingLength = arraysize(kDomainMapping);
+
+  GenerateRandomId();
+
+  GURL url(url_host);
+  for (size_t i = 0; i < kDomainMappingLength; ++i) {
+    std::string domain = kDomainMapping[i].domain_name;

[Ilya Sherman, 2013/09/12 04:45:45]

nit: Not a big deal, but this seems like an unnecessary string allocation +
copy, especially since you go right back and pull out the c-string below.

[jdomingos, 2013/09/12 12:01:01]

Done.

+    if (url.DomainIs(domain.c_str()))
+      return kDomainMapping[i].group_ids[g_random_group_id];
+  }
+  return 0;
+}
+
+void LogUMAHistogramEnumeration(const std::string& name,
+                                int sample,
+                                int boundary_value) {
+  DCHECK_LT(sample, boundary_value);
+
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::LinearHistogram::FactoryGet(
+          name,
+          1,
+          boundary_value,
+          boundary_value + 1,
+          base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->Add(sample);
+}
+
+void LogUMAHistogramCounts(const std::string& name, int sample) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::Histogram::FactoryGet(
+          name,
+          1,
+          1000000,
+          kGroupNumber,
+          base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->Add(sample);
+}
+
+void LogUMAHistogramBoolean(const std::string& name, bool sample) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::BooleanHistogram::FactoryGet(
+          name,
+          base::Histogram::kNoFlags);
+          histogram->AddBoolean(sample);
+}
+
+}  // namespace password_manager_metrics_util