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 dbeec6b27480552cb19fb5626680514dd9adb1d5..e47d8424acd08e06921706c7f1decdc807fa4a85 100644
--- a/chrome/browser/password_manager/password_manager.cc
+++ b/chrome/browser/password_manager/password_manager.cc
@@ -62,6 +62,21 @@ void ReportMetrics(bool password_manager_enabled) {
     content::RecordAction(UserMetricsAction("PasswordManager_Disabled"));
 }
 
+void ReportSavePasswordInfoBarFailure(
+    const enum PasswordManager::PotentialInfoBarErrorReasons reason_id,
+    const GURL& url) {
+  std::string domain_name = url.host();
+
+  if (domain_name.size())
+    domain_name.insert(0, "_");

[vabr (Chromium), 2013/08/14 14:39:58]

We may not append any domain name, only one of a carefully selected popular ones
(say top 5 or top 10 on the list from Patrick Nepper).
For other domains, we should not report at all.

[Garrett Casto, 2013/08/14 23:02:47]

I didn't even realize you could send any identifiable information at all via UMA
stats, though I trust your judgement. What is this list of domains BTW? Do we
think that it's going to be worth trying to report domain stats for just a few
domains? If we do this, will we have to update the privacy policy in any way?

[vabr (Chromium), 2013/08/16 09:02:14]

Yeah, we have to be careful. Logging the domain for a very popular site might
not be considered identifying, but this will still need a privacy review (by
someone else than me). There is already a review ticket for M31 waiting for
this: http://crbug.com/268980 (I Cc-ed you for access).

+
+  UMA_HISTOGRAM_BOOLEAN("PasswordManager.InfobarDisplayed" + domain_name,

[Garrett Casto, 2013/08/14 23:02:47]

I'm not sure if this is necessary as if this is negative, we are logging the
failure reasons, and if it's positive then we have a different UMA stat that
tracks that "PasswordManager.InfoBarResponse"

[vabr (Chromium), 2013/08/16 09:02:14]

That's a good point.

Jordy, when you are back, let's talk about how we would actually display the
signals you are adding, and see if having this one helps anything or not.

+                        false);
+  UMA_HISTOGRAM_ENUMERATION(
+      "PasswordManager.PotentialInfoBarErrorReasons" + domain_name, reason_id,
+      PasswordManager::NUM_ERROR_TYPES);
+}
+
 }  // namespace
 
 // static
@@ -137,12 +152,16 @@ bool PasswordManager::IsSavingEnabled() const {
 }
 
 void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
-  if (!IsSavingEnabled())
+  if (!IsSavingEnabled()) {

[Garrett Casto, 2013/08/14 23:02:47]

I actually just submitted a CL (https://codereview.chromium.org/22960002/) which
records provisional save failure cases, so you should probably sync and pick up
that CL. It's basically a subset of what you are trying to do, as it only checks
provisional save password failures and doesn't try to record any domain name
information.

+    ReportSavePasswordInfoBarFailure(SAVE_PASSWORD_DISABLE, form.origin);
     return;
+  }
 
   // No password to save? Then don't.
-  if (form.password_value.empty())
+  if (form.password_value.empty()) {
+    ReportSavePasswordInfoBarFailure(EMPTY_PASSWORD, form.origin);
     return;
+  }
 
   scoped_ptr<PasswordFormManager> manager;
   ScopedVector<PasswordFormManager>::iterator matched_manager_it =
@@ -173,6 +192,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
     manager.reset(*matched_manager_it);
     pending_login_managers_.weak_erase(matched_manager_it);
   } else {
+    ReportSavePasswordInfoBarFailure(FIRST_PAGE_NOT_LOADED, form.origin);
     return;
   }
 
@@ -180,23 +200,32 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   // tried to submit credentials before we had time to even find matching
   // results for the given form and autofill. If this is the case, we just
   // give up.
-  if (!manager->HasCompletedMatching())
+  if (!manager->HasCompletedMatching()) {
+    ReportSavePasswordInfoBarFailure(MATCHING_ONGOING, form.origin);
     return;
+  }
 
   // Also get out of here if the user told us to 'never remember' passwords for
   // this form.
-  if (manager->IsBlacklisted())
+  if (manager->IsBlacklisted()) {
+    ReportSavePasswordInfoBarFailure(NEVER_REMEMBER, form.origin);
     return;
+  }
 
   // Bail if we're missing any of the necessary form components.
-  if (!manager->HasValidPasswordForm())
+  if (!manager->HasValidPasswordForm()) {
+    ReportSavePasswordInfoBarFailure(INVALID_FORM, form.origin);
     return;
+  }
 
   // Always save generated passwords, as the user expresses explicit intent for
   // Chrome to manage such passwords. For other passwords, respect the
   // autocomplete attribute.
-  if (!manager->HasGeneratedPassword() && !form.password_autocomplete_set)
+  if (!manager->HasGeneratedPassword() && !form.password_autocomplete_set) {
+    ReportSavePasswordInfoBarFailure(PASSWORD_GENERATED_OR_AUTOCOMPLETED,
+                                     form.origin);
     return;
+  }
 
   PasswordForm provisionally_saved_form(form);
   provisionally_saved_form.ssl_valid = form.origin.SchemeIsSecure() &&
@@ -271,15 +300,34 @@ void PasswordManager::OnPasswordFormsParsed(
 }
 
 bool PasswordManager::ShouldShowSavePasswordInfoBar() const {

[Garrett Casto, 2013/08/14 23:02:47]

I don't think that this function does what you think. If it returns true it
means that we need to prompt the user to save the password, otherwise the user
has already given their consent and we can save automatically. Having stats on
these cases is interesting, but I'm not sure if we want to group them together
with cases where we actually don't save the password.

-  return provisional_save_manager_->IsNewLogin() &&
-      !provisional_save_manager_->HasGeneratedPassword() &&
-      !provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch();
+
+  // All functions are tested on by one in order to send the appropriate

[vabr (Chromium), 2013/08/14 14:39:58]

on by one -> one by one

+  // UMA signal with the ReportSavePasswordInfoBarFailure function.
+  if (!provisional_save_manager_->IsNewLogin()) {
+    ReportSavePasswordInfoBarFailure(LOGIN_ALREADY_KNEW,
+                                     GURL(provisional_save_manager_->realm()));
+    return false;
+  }
+  if (provisional_save_manager_->HasGeneratedPassword()) {
+    ReportSavePasswordInfoBarFailure(PASSWORD_GENERATED,
+                                     GURL(provisional_save_manager_->realm()));
+    return false;
+  }
+  if (provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch()) {
+    ReportSavePasswordInfoBarFailure(ORIGIN_MATCHING_OF_PUBLIC_SUFFIX,
+                                     GURL(provisional_save_manager_->realm()));
+    return false;
+  }
+  return true;
 }
 
 void PasswordManager::OnPasswordFormsRendered(
     const std::vector<PasswordForm>& visible_forms) {
-  if (!provisional_save_manager_.get())
+  if (!provisional_save_manager_.get()) {
+    ReportSavePasswordInfoBarFailure(CANNOT_GET_THE_PROVIOSIONAL_PASSWORD,
+                                     visible_forms[0].origin);

[Garrett Casto, 2013/08/14 23:02:47]

Not sure if any of the reporting in this function is necessary. 

This will get triggered only if provisional saving fails, which we already have
monitoring for.

     return;
+  }
 
   DCHECK(IsSavingEnabled());
 
@@ -290,6 +338,8 @@ void PasswordManager::OnPasswordFormsRendered(
         *iter, PasswordFormManager::ACTION_MATCH_REQUIRED)) {
       // The form trying to be saved has immediately re-appeared. Assume login
       // failure and abort this save, by clearing provisional_save_manager_.
+      ReportSavePasswordInfoBarFailure(
+          FORM_REAPPEARED, GURL(provisional_save_manager_->realm()));

[Garrett Casto, 2013/08/14 23:02:47]

We already keep track of this stat in the provisional_save_manager. See
PasswordFormManager::SubmitFailed()

       provisional_save_manager_->SubmitFailed();
       provisional_save_manager_.reset();
       return;
@@ -299,6 +349,8 @@ void PasswordManager::OnPasswordFormsRendered(
   if (!provisional_save_manager_->HasValidPasswordForm()) {
     // Form is not completely valid - we do not support it.
     NOTREACHED();
+    ReportSavePasswordInfoBarFailure(INVALID_FORM,
+                                     GURL(provisional_save_manager_->realm()));
     provisional_save_manager_.reset();
     return;
   }