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..0a77aefb853123933ddff05997c62e30de42e81f
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_metrics_util.cc
@@ -0,0 +1,97 @@
+// 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"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "url/gurl.h"
+
+namespace password_manager_metrics_util {
+
+std::string IsDomainNameMonitored(const std::string& url_host) {
+  // This array contains each monitored websites together with all id of
+  // buckets which contain the website.
+  const struct MonitoredWebsiteInfos kInfos[] = {

[vabr (Chromium), 2013/09/10 08:46:53]

This should be also "static", otherwise I believe the whole table is created on
the stack during each call to IsDomainNameMonitored.

[jdomingos, 2013/09/10 16:49:13]

Done.

+      {"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", {13, 7, 9, 10, 14, 15, 17, 18, 19, 20}},
+      {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}},
+  };
+  const size_t kInfosLength = arraysize(kInfos);
+
+  GURL url(url_host);
+  for (size_t i = 0; i < kInfosLength; ++i) {
+    std::string domain = kInfos[i].domain_name;
+    // For each session, the user uses only one bucket for each website.
+    if (url.DomainIs(domain.c_str())) {
+      return "_bucket" +
+             base::IntToString(
+                 kInfos[i].bucketsID[base::GetCurrentProcId() % kInfosLength]);

[vabr (Chromium), 2013/09/10 08:46:53]

Please do not use |kInfosLength| here!
The fact that both kInfosLength and the size of the bucketsID arrays is always
the same (10) is a coincidence. If anyone changed those parameters in the
future, we might overflow the array's limits.

Instead, please introduce also something like

// The common size of the bucket_ids part of each element in 
// |kInfosLength|.
static const size_t kBucketsPerDomain = 10u;

right below (or above) kInfosLength.

[vabr (Chromium), 2013/09/10 08:46:53]

Using GetCurrentProcId() should also have a comment explaining why we are using
this and what would an alternative need to satisfy. I suggest:

// Use GetCurrentProcId % kBucketsPerDomain to get a number which:
// 1. Is stable during the lifetime of the browser.
// 2. Is sufficiently random, in both the sense that
//   2a. it's not easy to predict for a specific user, and
//   2b. all kBucketsPerDomain possible values are roughly uniformly
//       used across all users.

[jdomingos, 2013/09/10 16:49:13]

Done.

[jdomingos, 2013/09/10 16:49:13]

Done.

+    }
+  }
+  return std::string();
+}
+
+void LogUMAHistogramEnumeration(const std::string& name,

[vabr (Chromium), 2013/09/10 08:46:53]

(Dear OWNERS, any guidance on the following?)

Looking at all the LogUMAHistogram* functions, I'm thinking how to avoid code
duplication (against the originals in
components/autofill/core/browser/autofill_metrics.cc).

You use these functions in a code from chrome/chrome_browser.gypi:browser, which
already lists a couple of dependencies from ../components/components.gyp:...
The originals of LogUMAHistogram* are in ../components/components.gyp:autofill.
You can either add that to the dependencies of
chrome/chrome_browser.gypi:browser, or separate those LogUMAHistogram* functions
in their own directory and target and only add that (see the
startup_metric_utils for an example).

[Ilya Sherman, 2013/09/10 22:33:47]

The most appropriate place for these would be in base/metrics, if we want to
reduce the amount of code duplication.

+                                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 LogUMAHistogramTimes(const std::string& name,
+                          const base::TimeDelta& duration) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::Histogram::FactoryTimeGet(
+          name,
+          base::TimeDelta::FromMilliseconds(1),
+          base::TimeDelta::FromSeconds(10),
+          50,
+          base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->AddTime(duration);
+}
+
+void LogUMAHistogramCount(const std::string& name, int sample) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::Histogram::FactoryGet(
+        name,
+        1,
+        1000000,
+        50,
+        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