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.h

Index: chrome/browser/password_manager/password_manager_metrics_util.h
diff --git a/chrome/browser/password_manager/password_manager_metrics_util.h b/chrome/browser/password_manager/password_manager_metrics_util.h
new file mode 100644
index 0000000000000000000000000000000000000000..d15a31d78cd333146b84a7893c2672c50459cd98
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_metrics_util.h
@@ -0,0 +1,58 @@
+// 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.
+
+#ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_METRICS_UTIL_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_METRICS_UTIL_H_
+
+#include <string>
+
+namespace base {
+class TimeDelta;
+}
+
+namespace password_manager_metrics_util {
+
+// The number of time that each monitored website appears.

[vabr (Chromium), 2013/09/11 17:31:24]

Please change
"time that" -> "groups in which"

[jdomingos, 2013/09/11 19:15:28]

Done.

+// It is a half of the total number of buckets.

[vabr (Chromium), 2013/09/11 17:31:24]

First: "groups", not "buckets".
Second: If you mean that this is always the half of kGroupNumber,
then don't say this here, and instead define
const size_t kGroupNumber = 2 * kGroupsPerDomain;
below.

[jdomingos, 2013/09/11 19:15:28]

Done.

+// For more information see goo.gl/vUuFd5.

[vabr (Chromium), 2013/09/11 17:31:24]

Please add http://, so that this becomes a clickable link in CodeSearch.

[vabr (Chromium), 2013/09/11 17:31:24]

Also, maybe write a very short paragraph about what are domains and groups in
this context, put it just after the namespace definition (with a trailing blank
line), and then move the link to the design doc at the end of that paragraph.

Suggestion for the paragraph text (but feel free to rephrase):
"We monitor the performance of the save password heuristic for a handful of
domains. For privacy reasons we are not reporting UMA signals by domain, but by
a domain group. A domain group can contain mutliple domains, and a domain can be
contained in multiple groups."


Also, having the preamble will make comments to individual definitions below
shorter and clearer.

[jdomingos, 2013/09/11 19:15:28]

Done.

[jdomingos, 2013/09/11 19:15:28]

Done.

+const size_t kGroupsPerDomain = 10u;

[vabr (Chromium), 2013/09/11 17:31:24]

Also, are these constants used outside of this header file's *.cc file? If not,
they should go to the .cc file and into an anonymous namespace.

+
+// Number of groups which can store monitored websites.

[vabr (Chromium), 2013/09/11 17:31:24]

"store monitored website" sounds confusing (like saving the HTML).
Maybe remove this comment, and instead add an initial comment for this section,
as described above.

[jdomingos, 2013/09/11 19:15:28]

Done.

+const size_t kGroupNumber = 20u;
+
+// Check whether the |url_host| is monitored or not. If yes, we return
+// the name of the group which contains the domain name otherwise
+// returns an empty string.
+std::string MonitoredDomainGroup(const std::string& url_host);
+
+// A version of the UMA_HISTOGRAM_ENUMERATION macro that allows the |name|
+// to vary over the program's runtime.
+void LogUMAHistogramEnumeration(const std::string& name,
+                                int sample,
+                                int boundary_value);
+
+// A version of the UMA_HISTOGRAM_TIMES macro that allows the |name|
+// to vary over the program's runtime.
+void LogUMAHistogramTimes(const std::string& name,
+                          const base::TimeDelta& duration);
+
+// A version of the UMA_HISTOGRAM_COUNTS macro that allows the |name|
+// to vary over the program's runtime.
+void LogUMAHistogramCounts(const std::string& name, int sample);
+
+// A version of the UMA_HISTOGRAM_BOOLEAN macro that allows the |name|
+// to vary over the program's runtime.
+void LogUMAHistogramBoolean(const std::string& name, bool sample);
+
+// 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();

[vabr (Chromium), 2013/09/11 17:31:24]

Please capitalise (GenerateRandomId), also below for SetRandomId.

[jdomingos, 2013/09/11 19:15:28]

Done.

+
+// Only used in the IsDomainNameMonitoredTest.
+void setRandomId(unsigned int value);

[vabr (Chromium), 2013/09/11 17:31:24]

Please append "ForTesting" to the function name. This marks is clearly as a
testing-only code. You can then drop the comment.

[jdomingos, 2013/09/11 19:15:28]

Done.

+
+}  // namespace password_manager_metrics_util
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_METRICS_UTIL_H_