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"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/platform_thread.h"
 #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"
 #include "chrome/common/pref_names.h"
 #include "components/autofill/core/common/autofill_messages.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
@@ skipping 21 matching lines @@
 void ReportMetrics(bool password_manager_enabled) {
   static base::PlatformThreadId initial_thread_id =
       base::PlatformThread::CurrentId();
   DCHECK(initial_thread_id == base::PlatformThread::CurrentId());
 
   static bool ran_once = false;
   if (ran_once)
     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
 
 // static
 void PasswordManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(
       prefs::kPasswordManagerEnabled,
       true,
@@ skipping 58 matching lines @@
   // 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()) {
-    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;
   }
 
   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
@@ skipping 12 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 {
-    RecordFailure(NO_MATCHING_FORM);
+    RecordFailure(NO_MATCHING_FORM, form.origin.host());
     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()) {
-    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;
   }
 
   // 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) {
-    RecordFailure(AUTOCOMPLETE_OFF);
+    RecordFailure(AUTOCOMPLETE_OFF, form.origin.host());
     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);
   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(
     const PasswordSubmittedCallback& callback) {
   submission_callbacks_.push_back(callback);
 }
 
 void PasswordManager::AddObserver(LoginModelObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ skipping 54 matching lines @@
                                 web_contents(),
                                 *iter,
                                 ssl_valid);
     pending_login_managers_.push_back(manager);
     manager->FetchMatchingLoginsFromPasswordStore();
   }
 }
 
 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(
     const std::vector<PasswordForm>& visible_forms) {
   if (!provisional_save_manager_.get())
     return;
 
   DCHECK(IsSavingEnabled());
 
   // First, check for a failed login attempt.
@@ skipping 104 matching lines @@
       return;
     }
     default:
       FOR_EACH_OBSERVER(
           LoginModelObserver,
           observers_,
           OnAutofillDataAvailable(preferred_match.username_value,
                                   preferred_match.password_value));
   }
 }