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

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/password_manager/password_manager.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 44 matching lines @@
   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"));
   else
     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
 void PasswordManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(
       prefs::kPasswordManagerEnabled,
       true,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterBooleanPref(
@@ skipping 55 matching lines @@
   manager->SetHasGeneratedPassword();
   // TODO(gcasto): Add UMA stats to track this.
 }
 
 bool PasswordManager::IsSavingEnabled() const {
   return *password_manager_enabled_ &&
          !delegate_->GetProfile()->IsOffTheRecord();
 }
 
 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 =
       pending_login_managers_.end();
   for (ScopedVector<PasswordFormManager>::iterator iter =
            pending_login_managers_.begin();
        iter != pending_login_managers_.end(); ++iter) {
     // If we find a manager that exactly matches the submitted form including
     // the action URL, exit the loop.
     if ((*iter)->DoesManage(
@@ skipping 10 matching lines @@
   }
   // If we didn't find a manager, this means a form was submitted without
   // first loading the page containing the form. Don't offer to save
   // passwords in this case.
   if (matched_manager_it != pending_login_managers_.end()) {
     // Transfer ownership of the manager from |pending_login_managers_| to
     // |manager|.
     manager.reset(*matched_manager_it);
     pending_login_managers_.weak_erase(matched_manager_it);
   } else {
+    ReportSavePasswordInfoBarFailure(FIRST_PAGE_NOT_LOADED, form.origin);
     return;
   }
 
   // If we found a manager but it didn't finish matching yet, the user has
   // 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() &&
       !delegate_->DidLastPageLoadEncounterSSLErrors();
   provisionally_saved_form.preferred = true;
   PasswordFormManager::OtherPossibleUsernamesAction action =
       PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES;
   if (OtherPossibleUsernamesEnabled())
     action = PasswordFormManager::ALLOW_OTHER_POSSIBLE_USERNAMES;
   manager->ProvisionallySave(provisionally_saved_form, action);
@@ skipping 53 matching lines @@
         new PasswordFormManager(delegate_->GetProfile(),
                                 this,
                                 web_contents(),
                                 *iter,
                                 ssl_valid);
     pending_login_managers_.push_back(manager);
     manager->FetchMatchingLoginsFromPasswordStore();
   }
 }
 
 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());
 
   // First, check for a failed login attempt.
   for (std::vector<PasswordForm>::const_iterator iter = visible_forms.begin();
        iter != visible_forms.end(); ++iter) {
     if (provisional_save_manager_->DoesManage(
         *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;
     }
   }
 
   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;
   }
 
   // Looks like a successful login attempt. Either show an infobar or
   // automatically save the login data. We prompt when the user hasn't already
   // given consent, either through previously accepting the infobar or by having
   // the browser generate the password.
   provisional_save_manager_->SubmitPassed();
   if (provisional_save_manager_->HasGeneratedPassword())
@@ skipping 74 matching lines @@
       return;
     }
     default:
       FOR_EACH_OBSERVER(
           LoginModelObserver,
           observers_,
           OnAutofillDataAvailable(preferred_match.username_value,
                                   preferred_match.password_value));
   }
 }