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

 // 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_delegate_impl.h"
 
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.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_metrics_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"
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/common/autofill_messages.h"
 #include "components/autofill/core/common/password_form.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/ssl_status.h"
@@ skipping 43 matching lines @@
 
   virtual InfoBarAutomationType GetInfoBarAutomationType() const OVERRIDE;
 
   // The PasswordFormManager managing the form we're asking the user about,
   // and should update as per her decision.
   scoped_ptr<PasswordFormManager> form_to_save_;
 
   // 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 group name corresponding to the domain name of
+  // |form_to_save_| if the form is on a monitored domain.
+  // Otherwise, an empty string.
+  std::string uma_histogram_suffix_;
+
   DISALLOW_COPY_AND_ASSIGN(SavePasswordInfoBarDelegate);
 };
 
 // static
 void SavePasswordInfoBarDelegate::Create(content::WebContents* web_contents,
                                          PasswordFormManager* form_to_save) {
 #if defined(ENABLE_ONE_CLICK_SIGNIN)
   // Don't show the password manager infobar if this form is for a google
   // account and we are going to show the one-click signin infobar.
   GURL realm(form_to_save->realm());
@@ skipping 12 matching lines @@
   infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>(
       new SavePasswordInfoBarDelegate(infobar_service, form_to_save)));
 }
 
 SavePasswordInfoBarDelegate::SavePasswordInfoBarDelegate(
     InfoBarService* infobar_service,
     PasswordFormManager* form_to_save)
     : ConfirmInfoBarDelegate(infobar_service),
       form_to_save_(form_to_save),
       infobar_response_(NO_RESPONSE) {
+  uma_histogram_suffix_ = password_manager_metrics_util::MonitoredDomainGroup(

[vabr (Chromium), 2013/09/11 17:31:24]

nit: this could be done in the initialiser list:

uma_histogram_suffix_(password_manager_metrics_util::MonitoredDomainGroup(...))

Then you can make uma_histogram_suffix_ a const member variable, improving the
readability and efficiency.

[jdomingos, 2013/09/11 19:15:28]

Done.

+      form_to_save->realm());
+  if (!uma_histogram_suffix_.empty()) {
+    int group_number = 0;
+    std::string string_group_number =
+        uma_histogram_suffix_.substr(uma_histogram_suffix_.find("_") + 1);
+
+    if (!base::StringToInt(string_group_number, &group_number))

[vabr (Chromium), 2013/09/11 17:31:24]

You have just written an int to a string in MonitoredDomainGroup, and here you
parse it back again. Could you get it directly? For example, you can split
MonitoredDomainGroup into the part which gets the number, and the wrapper which
creates the suffix, and call here only the inner part.

[jdomingos, 2013/09/11 19:15:28]

Done.

+      return;
+    password_manager_metrics_util::LogUMAHistogramCounts(
+        "PasswordManager.SavePasswordPromptDisplayed",
+        group_number);
+  }
 }
 
 SavePasswordInfoBarDelegate::~SavePasswordInfoBarDelegate() {
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.InfoBarResponse",
                             infobar_response_, NUM_RESPONSE_TYPES);
+  // Time which allows us to consider that the info bar has been
+  // displayed during enough time for the user to make a decision.

[vabr (Chromium), 2013/09/11 17:31:24]

This is not grammatically correct, and could me more informative.
Suggested rephrasing:

"The shortest period (in ms) for which the prompt needs to
live, so that we don't consider it killed prematurely, as might
happen, e.g., if a pre-rendered page gets swapped in (and the
current WebContent is destroyed)."

[jdomingos, 2013/09/11 19:15:28]

Done.

+  const int kTime = 1000;
+
+  if (!uma_histogram_suffix_.empty()) {
+    password_manager_metrics_util::LogUMAHistogramEnumeration(
+        "PasswordManager.SavePasswordPromptResponse_" + uma_histogram_suffix_,
+        infobar_response_, NUM_RESPONSE_TYPES);
+    password_manager_metrics_util::LogUMAHistogramBoolean(
+        "PasswordManager.SavePasswordPromptDisappearedQuickly" +
+        uma_histogram_suffix_,
+        timer_.Elapsed().InMilliseconds() < kTime);
+  }
 }
 
 int SavePasswordInfoBarDelegate::GetIconID() const {
   return IDR_INFOBAR_SAVE_PASSWORD;
 }
 
 InfoBarDelegate::Type SavePasswordInfoBarDelegate::GetInfoBarType() const {
   return PAGE_ACTION_TYPE;
 }
 
@@ skipping 59 matching lines @@
 bool PasswordManagerDelegateImpl::DidLastPageLoadEncounterSSLErrors() {
   content::NavigationEntry* entry =
       web_contents_->GetController().GetActiveEntry();
   if (!entry) {
     NOTREACHED();
     return false;
   }
 
   return net::IsCertStatusError(entry->GetSSL().cert_status);
 }