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

Index: chrome/browser/password_manager/password_manager.cc
diff --git a/chrome/browser/password_manager/password_manager.cc b/chrome/browser/password_manager/password_manager.cc
index 39629e94fc51903a40d3d6e14730e8f9d61eccbb..8f92b3f45642183236a6b0628f1b17c494676e26 100644
--- a/chrome/browser/password_manager/password_manager.cc
+++ b/chrome/browser/password_manager/password_manager.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/browser/password_manager/password_manager_delegate.h"
+#include "chrome/browser/password_manager/password_manager_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
@@ -55,12 +56,10 @@ void ReportMetrics(bool password_manager_enabled) {
     return;
   ran_once = true;
 
-  // TODO(isherman): This does not actually measure a user action.  It should be
-  // a boolean histogram.
   if (password_manager_enabled)
-    content::RecordAction(UserMetricsAction("PasswordManager_Enabled"));
+    UMA_HISTOGRAM_BOOLEAN("PasswordManager.PasswordManager_Enabled", true);
   else
-    content::RecordAction(UserMetricsAction("PasswordManager_Disabled"));
+    UMA_HISTOGRAM_BOOLEAN("PasswordManager.PasswordManager_Enabled", false);

[Garrett Casto, 2013/08/28 01:09:17]

I would think that the name should be "PasswordManager.Enabled". Also, you don't
need an if clause, just use |password_manager_enabled| in
UMA_HISTOGRAM_BOOLEAN(...);

 }
 
 }  // namespace
@@ -139,13 +138,13 @@ bool PasswordManager::IsSavingEnabled() const {
 
 void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   if (!IsSavingEnabled()) {
-    RecordFailure(SAVING_DISABLED);
+    RecordFailure(SAVING_DISABLED, form.origin.host());
     return;
   }
 
   // No password to save? Then don't.
   if (form.password_value.empty()) {
-    RecordFailure(EMPTY_PASSWORD);
+    RecordFailure(EMPTY_PASSWORD, form.origin.host());
     return;
   }
 
@@ -178,7 +177,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
     manager.reset(*matched_manager_it);
     pending_login_managers_.weak_erase(matched_manager_it);
   } else {
-    RecordFailure(NO_MATCHING_FORM);
+    RecordFailure(NO_MATCHING_FORM, form.origin.host());
     return;
   }
 
@@ -187,20 +186,20 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   // results for the given form and autofill. If this is the case, we just
   // give up.
   if (!manager->HasCompletedMatching()) {
-    RecordFailure(MATCHING_NOT_COMPLETE);
+    RecordFailure(MATCHING_NOT_COMPLETE, form.origin.host());
     return;
   }
 
   // Also get out of here if the user told us to 'never remember' passwords for
   // this form.
   if (manager->IsBlacklisted()) {
-    RecordFailure(FORM_BLACKLISTED);
+    RecordFailure(FORM_BLACKLISTED, form.origin.host());
     return;
   }
 
   // Bail if we're missing any of the necessary form components.
   if (!manager->HasValidPasswordForm()) {
-    RecordFailure(INVALID_FORM);
+    RecordFailure(INVALID_FORM, form.origin.host());
     return;
   }
 
@@ -208,7 +207,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   // Chrome to manage such passwords. For other passwords, respect the
   // autocomplete attribute.
   if (!manager->HasGeneratedPassword() && !form.password_autocomplete_set) {
-    RecordFailure(AUTOCOMPLETE_OFF);
+    RecordFailure(AUTOCOMPLETE_OFF, form.origin.host());
     return;
   }
 
@@ -224,9 +223,18 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   provisional_save_manager_.swap(manager);
 }
 
-void PasswordManager::RecordFailure(ProvisionalSaveFailure failure) {
+void PasswordManager::RecordFailure(ProvisionalSaveFailure failure,
+                                    const std::string& url_host) {
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.ProvisionalSaveFailure",
                             failure, MAX_FAILURE_VALUE);
+
+  std::string domain_name;
+  if (PasswordManagerUtil::IsDomainNameMonitored(url_host, domain_name)) {
+    domain_name.insert(0, "_");
+    UMA_HISTOGRAM_ENUMERATION(

[Garrett Casto, 2013/08/28 01:09:17]

Unfortunately this doesn't work. If you check out the expansion of
UMA_HISTOGRAM_ENUMERATION, it believes the name of the histogram is constant and
will store it in a static variable. Read the comments at the top of histogram.h
for more information.

I'm actually not sure if there is a reasonable way to dynamically generate
histogram names, though Ilya might know.

[jdomingos, 2013/08/28 15:23:20]

Normally I think it should work because I added a fieldtrial which permit me to
append "group name" to each histogram which are listed as "affected-histogram
name".
https://codereview.chromium.org/23140005/diff/25001/tools/metrics/histograms/....

Am I wrong ?

[Garrett Casto, 2013/08/28 19:15:19]

I think you pasted the wrong link.

Do you mean you did something like
UMA_HISTOGRAM_COUNTS(FieldTrial::MakeName("PasswordManager.", "Experiment"),
count);

This still works because field trials are static across a given run, but in the
cases that you have the domain name can change during a run. You can test it out
to verify (I think that it will DCHECK()) but it looks like it will fail.

+        "PasswordManager.ProvisionalSaveFailure" + domain_name, failure,
+        MAX_FAILURE_VALUE);
+  }
 }
 
 void PasswordManager::AddSubmissionCallback(
@@ -301,8 +309,8 @@ void PasswordManager::OnPasswordFormsParsed(
 
 bool PasswordManager::ShouldShowSavePasswordInfoBar() const {
   return provisional_save_manager_->IsNewLogin() &&
-      !provisional_save_manager_->HasGeneratedPassword() &&
-      !provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch();
+         !provisional_save_manager_->HasGeneratedPassword() &&
+         !provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch();
 }
 
 void PasswordManager::OnPasswordFormsRendered(