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;
+}
+
+namespace password_manager_metrics_util {
+
+struct DomainBucketsPair {
+  char const* domain_name;

[Ilya Sherman, 2013/09/10 22:30:54]

char const* char

[vabr (Chromium), 2013/09/11 07:25:43]

I suppose Ilya meant
const char*
?

[Ilya Sherman, 2013/09/11 23:33:04]

Whoops, good catch.  I meant "const char* const".

+  int const bucket_ids[10];

[Ilya Sherman, 2013/09/10 22:30:54]

Why does each domain have 10 bucket ids?

[vabr (Chromium), 2013/09/11 07:25:43]

int const -> const int

[vabr (Chromium), 2013/09/11 07:25:43]

It has to be a half of the total number of buckets.
For that we chose 20 rather randomly (choosing less means harder time
interpreting the results, choosing more means more histograms and more values to
monitor).
This parameter should be documented in the design doc as well, before we land
this (it's not yet).

[vabr (Chromium), 2013/09/11 07:25:43]

Jordy -- instead of just writing "10", you should introduce a named constant.
You can then
1. Give a short comment about why 10, pointing to the design doc.
2. Use the constant for for loops in the cc file.

[jdomingos, 2013/09/11 15:50:30]

Done.

+};

[Ilya Sherman, 2013/09/10 22:30:54]

This doesn't seem to be used anywhere else in the header file, so please move it
into the implementation file.

[Ilya Sherman, 2013/09/10 22:30:54]

Please document this struct.

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Done.

+
+// 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 IsDomainNameMonitored(const std::string& url_host);

[Ilya Sherman, 2013/09/10 22:30:54]

This method name suggests that the method returns a boolean, which is no longer
true.  Please update the name to match the current semantics.

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Done.

+
+// 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_COUNT macro that allows the |name|

[Ilya Sherman, 2013/09/10 22:30:54]

nit: I think you mean UMA_HISTOGRAM_COUNTS.  Please fix that here and below.

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Done.

+// to vary over the program's runtime.
+void LogUMAHistogramCount(const std::string& name, int sample);
+
+}  // namespace password_manager_metrics_util
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_METRICS_UTIL_H_