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_delegate_impl.cc

Index: chrome/browser/password_manager/password_manager_delegate_impl.cc
diff --git a/chrome/browser/password_manager/password_manager_delegate_impl.cc b/chrome/browser/password_manager/password_manager_delegate_impl.cc
index f7c42bf88110d2718864d978996f9d8f2010a8d7..16fd4677307fc719c859b256786895d7063cb180 100644
--- a/chrome/browser/password_manager/password_manager_delegate_impl.cc
+++ b/chrome/browser/password_manager/password_manager_delegate_impl.cc
@@ -6,11 +6,14 @@
 
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/perftimer.h"

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

I just noticed that the timer you're relying on is intended for test code. 
Please pick a different sort of timer, that is intended for production code.

[vabr (Chromium), 2013/09/12 08:51:04]

That's a good point. Probably something from base/timer/timer.h would be
appropriate.

As a side point, the PerfTimer seems to be used in a couple of places in
production code, so I guess I will ask at chromium-dev why is that timer in
test, and whether we should correct the current usage.

[vabr (Chromium), 2013/09/12 10:12:22]

Using timer/timer.h is actually a bad idea (thanks Jordy for pointing that out).
That timer is for scheduling, not measuring.

I'm not sure whether there is a production code timer for measuring. It looks
like PerfTimer is used for that purpose. I already e-mailed chromium-dev asking
about this, let's see whether we have any response.

[vabr (Chromium), 2013/09/12 13:40:02]

So the result from chromium-dev is that Thiago is working on moving PerfTimer
out of test: https://codereview.chromium.org/23985006/.
So it should be OK to keep using PerfTimer here.

 #include "chrome/browser/infobars/confirm_infobar_delegate.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/browser/password_manager/password_manager.h"
+#include "chrome/browser/password_manager/password_manager_metrics_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/sync/one_click_signin_helper.h"
 #include "components/autofill/content/browser/autofill_driver_impl.h"
@@ -74,6 +77,13 @@ class SavePasswordInfoBarDelegate : public ConfirmInfoBarDelegate {
   // Used to track the results we get from the info bar.
   ResponseType infobar_response_;
 
+  // Save password prompt lifetime needed for a UMA signal.
+  PerfTimer timer_;
+
+  // The group name corresponding to the domain name of |form_to_save_| if the
+  // form is on a monitored domain. Otherwise, an empty string.
+  const std::string uma_histogram_suffix_;
+
   DISALLOW_COPY_AND_ASSIGN(SavePasswordInfoBarDelegate);
 };
 
@@ -105,12 +115,40 @@ SavePasswordInfoBarDelegate::SavePasswordInfoBarDelegate(
     PasswordFormManager* form_to_save)
     : ConfirmInfoBarDelegate(infobar_service),
       form_to_save_(form_to_save),
-      infobar_response_(NO_RESPONSE) {
+      infobar_response_(NO_RESPONSE),
+      uma_histogram_suffix_(password_manager_metrics_util::AppendGroup(
+          password_manager_metrics_util::MonitoredDomainGroupId(
+              form_to_save->realm()))) {
+  if (!uma_histogram_suffix_.empty()) {
+    if (password_manager_metrics_util::MonitoredDomainGroupId(
+            form_to_save->realm()))
+      return;

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

What is the purpose of this early return, i.e. why do you need an additional
check beyond "if (!uma_histogram_suffix_.empty())"?  This check currently looks
wrong to me -- it will cause the constructor to return early unless the
monitored domain group id is 0...

[vabr (Chromium), 2013/09/12 08:51:04]

I agree, this code should be removed:
if
(password_manager_metrics_util::MonitoredDomainGroupId(form_to_save->realm()))
  return;

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

Done.

+    unsigned int group_id =
+        password_manager_metrics_util::MonitoredDomainGroupId(
+            form_to_save->realm());
+    password_manager_metrics_util::LogUMAHistogramCounts(
+        "PasswordManager.SavePasswordPromptDisplayed", group_id);

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

Please emit this as a linear (i.e. enumerated) histogram with exactly the number
of buckets that you're interested in.  Currently, this will allocate too many
buckets, and also place multiple groups into a single bucket.  (You can see what
I mean by looking at the values emitted to this histogram when viewed via
about:histograms.)

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

Done.

+  }
 }
 
 SavePasswordInfoBarDelegate::~SavePasswordInfoBarDelegate() {
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.InfoBarResponse",
                             infobar_response_, NUM_RESPONSE_TYPES);

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

nit: Please leave a blank line after this one.

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

Done.

+  // The shortest period (in ms) for which the prompt needs to
+  // live, so that we don't consider it killed prematurely, as might
+  // happen, e.g., if a pre-rendered page gets swapped in (and the
+  // current WebContent is destroyed)

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

nit: Please end the sentence with a period.

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

nit: WebContent -> WebContents

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

Done.

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

Done.

+  const int kTime = 1000;

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

Please give this constant an even more descriptive name.  A couple of
suggestions: kMinimumPromptDisplayTimeMs, kQuickDismissalThresholdMs.  Note the
"ms" suffix to indicate the units -- though even better would be to declare this
as a TimeDelta rather than as a raw integer.

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

Done.

+
+  if (!uma_histogram_suffix_.empty()) {
+    password_manager_metrics_util::LogUMAHistogramEnumeration(
+        "PasswordManager.SavePasswordPromptResponse_" + uma_histogram_suffix_,
+        infobar_response_, NUM_RESPONSE_TYPES);
+    password_manager_metrics_util::LogUMAHistogramBoolean(
+        "PasswordManager.SavePasswordPromptDisappearedQuickly" +
+        uma_histogram_suffix_,
+        timer_.Elapsed().InMilliseconds() < kTime);
+  }
 }
 
 int SavePasswordInfoBarDelegate::GetIconID() const {