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 95ce6b81ab845a42af0b2c5712b4be28dbc1b363..65fa15c9fee3fecef18e196c8751d93da46f49b1 100644
--- a/chrome/browser/password_manager/password_manager_delegate_impl.cc
+++ b/chrome/browser/password_manager/password_manager_delegate_impl.cc
@@ -7,10 +7,12 @@
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/perftimer.h"
 #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_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 +76,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 domain name of the current webpage. This variable is used for UMA
+  // signals.

[Ilya Sherman, 2013/08/29 06:42:21]

This comment doesn't fully reflect the use of the variable, neither in terms of
mentioning that it is empty for most domains, nor in mentioning that when it is
non-empty, it is prefixed with an underscore.

[jdomingos, 2013/08/30 21:09:20]

Done.

+  std::string domain_name_;
+
   DISALLOW_COPY_AND_ASSIGN(SavePasswordInfoBarDelegate);
 };
 
@@ -106,11 +115,22 @@ SavePasswordInfoBarDelegate::SavePasswordInfoBarDelegate(
     : ConfirmInfoBarDelegate(infobar_service),
       form_to_save_(form_to_save),
       infobar_response_(NO_RESPONSE) {
+  password_manager_util::IsDomainNameMonitored(form_to_save->realm(),
+                                               &domain_name_);
+  if (domain_name_.size())
+    domain_name_.insert(0, "_");
+  UMA_HISTOGRAM_BOOLEAN("PasswordManager.InfobarDisplayed" + domain_name_,
+                        true);

[Ilya Sherman, 2013/08/29 06:42:21]

This will not work, for the reasons that the other reviewers mentioned: The
UMA_HISTOGRAM macros require the histogram name to be a run-time static
constant.  Instead, you'll want to inline the macro, similarly to this:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[Ilya Sherman, 2013/08/29 06:42:21]

This is a little strange, because it causes the meaning of the
PasswordManager.InfobarDisplayed histogram to be "whether an infobar was
displayed on any website *other* than the ones we are measuring explicitly". 
IMO you should either always log the PasswordManager.InfobarDisplayed metric, or
add a suffix like "_other" for non-tracked domains.

Better yet, rather than logging this as N separate histograms, I'd recommend
logging this a single enumerated histogram, where each bucket corresponds to a
specific tracked domain, plus an additional bucket for "other".

[jdomingos, 2013/08/30 21:09:20]

Hi Ilya,

Indeed regarding the PasswordManager.InfobarDisplayed, we just want this
information for monitored site. I moved the UMA_HISTOGRAM_BOOLEAN in the if
block.

Regarding the N separate histograms, this will work for this signal but not for
the other ones present in the fieldtrial because they are already enumeration.

[jdomingos, 2013/08/30 21:09:20]

Done.

 }
 
 SavePasswordInfoBarDelegate::~SavePasswordInfoBarDelegate() {
+  // Time which allows us to consider that the info bar has been
+  // displayed during enough time for the user to make a decision.

[Ilya Sherman, 2013/08/29 06:42:21]

I don't understand what this comment means.  Could you please try re-wording it
to be clearer?  It also looks like it should be moved down, so that it is
clearly describing the UMA_HISTOGRAM_TIMES macro rather than the _ENUMERATION
macro.

[jdomingos, 2013/08/30 21:09:20]

The comment here was for a previous variable which I deleted. Sorry for it.

   UMA_HISTOGRAM_ENUMERATION("PasswordManager.InfoBarResponse",
                             infobar_response_, NUM_RESPONSE_TYPES);
+  UMA_HISTOGRAM_TIMES(
+      "PasswordManager.InfobarLifetime" + domain_name_,

[Ilya Sherman, 2013/08/29 06:42:21]

As above, you'll need to inline the macro, because the histogram name is not a
run-time constant.

[Ilya Sherman, 2013/08/29 06:42:21]

As above, it's strange that the un-suffixed PasswordManager.InfobarLifetime
histogram does not measure *all* sites, but only those that we are not tracking
explicitly via other histograms.

[jdomingos, 2013/08/30 21:09:20]

You are right, as above this is not the expected behavior.

[jdomingos, 2013/08/30 21:09:20]

Done.

+      timer_.Elapsed());
 }
 
 int SavePasswordInfoBarDelegate::GetIconID() const {