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

+// 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;
+}

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

nit: No longer needed?

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

Done.

+
+namespace password_manager_metrics_util {
+
+// 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

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

nit: "mutliple" -> "multiple"

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

Done.

+// can be contained in multiple groups.
+// For more information see http://goo.gl/vUuFd5.
+
+// The number of group in which each monitored website appears.
+// It is a half of the total number of groups.
+const size_t kGroupsPerDomain = 10u;
+
+// Check whether the |url_host| is monitored or not. If yes, we return
+// the id of the group which contains the domain name otherwise
+// returns -1.

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

What does it mean for a function that returns an unsigned integer to return -1?

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

Sorry the previous version used an integer.
Now the function returns 0 when the host is not found because the ids begin at
1.

+unsigned int MonitoredDomainGroupId(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_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.

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

|random_group_id| and |random_id| are not variable names, so they shouldn't be
surrounded by pipes, nor written in hacker_case.  Perhaps you meant
|g_random_group_id| in both cases?

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

This sentence seems like it describes the opposite of what actually happens in
the code.

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

Done.

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

Done.

+void GenerateRandomId();

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

This seems like it should be an internal method, tucked into the anonymous
namespace in the implementation file.

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

Done.

+
+void SetRandomIdForTesting(unsigned int value);
+
+// Returns a string which contains group_|group_id|
+std::string AppendGroup(unsigned int group_id);

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

nit: IMO this function would be better named something like "GroupIdToString()".

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

Done.

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