Merge to branch 2490 [Password Generation] Add UMA stats for non-gene…

…rated submission rates

This allows for apples to apples comparison of generated vs. non-generated
submission success rates on the same forms.

BUG=509827

Review URL: https://codereview.chromium.org/1228783006

Cr-Commit-Position: refs/heads/master@{#345707}
(cherry picked from commit cc48d84)

Review URL: https://codereview.chromium.org/1320713006 .

Cr-Commit-Position: refs/branch-heads/2490@{#190}
Cr-Branched-From: 7790a35-refs/heads/master@{#344925}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -412,6 +412,8 @@ bool ChromePasswordManagerClient::OnMessageReceived(
     IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
     IPC_MESSAGE_HANDLER(AutofillHostMsg_HidePasswordGenerationPopup,
                         HidePasswordGenerationPopup)
+    IPC_MESSAGE_HANDLER(AutofillHostMsg_GenerationAvailableForForm,
+                        GenerationAvailableForForm)
     IPC_MESSAGE_HANDLER(AutofillHostMsg_PasswordAutofillAgentConstructed,
                         NotifyRendererOfLoggingAvailability)
     // Default:
@@ -460,6 +462,11 @@ void ChromePasswordManagerClient::ShowPasswordEditingPopup(
   popup_controller_->Show(false /* display_password */);
 }
 
+void ChromePasswordManagerClient::GenerationAvailableForForm(
+    const autofill::PasswordForm& form) {
+  password_manager_.GenerationAvailableForForm(form);
+}
+
 void ChromePasswordManagerClient::NotifyRendererOfLoggingAvailability() {
   if (!web_contents())
     return;

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -132,6 +132,10 @@ class ChromePasswordManagerClient
                                 const gfx::RectF& bounds,
                                 const autofill::PasswordForm& form);
 
+  // Notify the PasswordManager that generation is available for |form|. Used
+  // for UMA stats.
+  void GenerationAvailableForForm(const autofill::PasswordForm& form);
+
   // Sends a message to the renderer with the current value of
   // |can_use_log_router_|.
   void NotifyRendererOfLoggingAvailability();

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

@@ -1801,7 +1801,8 @@ TEST_F(PasswordAutofillAgentTest, PasswordGenerationSupersedesAutofill) {
   EXPECT_EQ(nullptr,
             render_thread_->sink().GetFirstMessageMatching(
                 AutofillHostMsg_ShowPasswordSuggestions::ID));
-  ExpectPasswordGenerationAvailable(password_generation_, true);
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
 }
 
 // Tests that a password change form is properly filled with the username and

chrome/renderer/autofill/password_generation_agent_browsertest.cc

@@ -59,8 +59,15 @@ class PasswordGenerationAgentTest : public ChromeRenderViewTest {
   void ExpectGenerationAvailable(const char* element_id,
                                  bool available) {
     FocusField(element_id);
-    ExpectPasswordGenerationAvailable(password_generation_,
-                                      available);
+    const IPC::Message* message =
+        render_thread_->sink().GetFirstMessageMatching(
+            AutofillHostMsg_ShowPasswordGenerationPopup::ID);
+    if (available)
+      ASSERT_TRUE(message);
+    else
+      ASSERT_FALSE(message);
+
+    render_thread_->sink().ClearMessages();
   }
 
  private:
@@ -300,7 +307,7 @@ TEST_F(PasswordGenerationAgentTest, EditingTest) {
   EXPECT_EQ(edited_password, second_password_element.value());
 
   // Clear any uninteresting sent messages.
-  password_generation_->clear_messages();
+  render_thread_->sink().ClearMessages();
 
   // Verify that password mirroring works correctly even when the password
   // is deleted.
@@ -310,11 +317,10 @@ TEST_F(PasswordGenerationAgentTest, EditingTest) {
 
   // Should have notified the browser that the password is no longer generated
   // and trigger generation again.
-  ASSERT_EQ(2u, password_generation_->messages().size());
-  EXPECT_EQ(AutofillHostMsg_PasswordNoLongerGenerated::ID,
-            password_generation_->messages()[0]->type());
-  EXPECT_EQ(AutofillHostMsg_ShowPasswordGenerationPopup::ID,
-            password_generation_->messages()[1]->type());
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_PasswordNoLongerGenerated::ID));
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
 }
 
 TEST_F(PasswordGenerationAgentTest, BlacklistedTest) {
@@ -393,42 +399,39 @@ TEST_F(PasswordGenerationAgentTest, MaximumOfferSize) {
       &first_password_element,
       std::string(password_generation_->kMaximumOfferSize - 1, 'a'));
   // There should now be a message to show the UI.
-  ASSERT_EQ(1u, password_generation_->messages().size());
-  EXPECT_EQ(AutofillHostMsg_ShowPasswordGenerationPopup::ID,
-            password_generation_->messages()[0]->type());
-  password_generation_->clear_messages();
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
+  render_thread_->sink().ClearMessages();
 
   // Simulate a user typing a password just over maximum offer size.
   SimulateUserTypingASCIICharacter('a', false);
   SimulateUserTypingASCIICharacter('a', true);
   // There should now be a message to hide the UI.
-  ASSERT_EQ(1u, password_generation_->messages().size());
-  EXPECT_EQ(AutofillHostMsg_HidePasswordGenerationPopup::ID,
-            password_generation_->messages()[0]->type());
-  password_generation_->clear_messages();
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_HidePasswordGenerationPopup::ID));
+  render_thread_->sink().ClearMessages();
 
   // Simulate the user deleting characters. The generation popup should be shown
   // again.
   SimulateUserTypingASCIICharacter(ui::VKEY_BACK, true);
   // There should now be a message to show the UI.
-  ASSERT_EQ(1u, password_generation_->messages().size());
-  EXPECT_EQ(AutofillHostMsg_ShowPasswordGenerationPopup::ID,
-            password_generation_->messages()[0]->type());
-  password_generation_->clear_messages();
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
+  render_thread_->sink().ClearMessages();
 
   // Change focus. Bubble should be hidden, but that is handled by AutofilAgent,
   // so no messages are sent.
   ExecuteJavaScriptForTests("document.getElementById('username').focus();");
-  EXPECT_EQ(0u, password_generation_->messages().size());
-  password_generation_->clear_messages();
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
+  render_thread_->sink().ClearMessages();
 
   // Focusing the password field will bring up the generation UI again.
   ExecuteJavaScriptForTests(
       "document.getElementById('first_password').focus();");
-  ASSERT_EQ(1u, password_generation_->messages().size());
-  EXPECT_EQ(AutofillHostMsg_ShowPasswordGenerationPopup::ID,
-            password_generation_->messages()[0]->type());
-  password_generation_->clear_messages();
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
+  render_thread_->sink().ClearMessages();
 
   // Loading a different page triggers UMA stat upload. Verify that only one
   // display event is sent even though
@@ -512,6 +515,7 @@ TEST_F(PasswordGenerationAgentTest, MessagesAfterAccountSignupFormFound) {
 
 // Losing focus should not trigger a password generation popup.
 TEST_F(PasswordGenerationAgentTest, BlurTest) {
+  render_thread_->sink().ClearMessages();
   LoadHTMLWithUserGesture(kDisabledElementAccountCreationFormHTML);
   SetNotBlacklistedMessage(password_generation_,
                            kDisabledElementAccountCreationFormHTML);
@@ -526,7 +530,10 @@ TEST_F(PasswordGenerationAgentTest, BlurTest) {
   // Remove focus from everywhere by clicking an unfocusable element: password
   // generation popup should not show up.
   EXPECT_TRUE(SimulateElementClick("disabled"));
-  EXPECT_EQ(0u, password_generation_->messages().size());
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_GenerationAvailableForForm::ID));
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      AutofillHostMsg_ShowPasswordGenerationPopup::ID));
 }
 
 TEST_F(PasswordGenerationAgentTest, AutocompleteAttributesTest) {

chrome/renderer/autofill/password_generation_test_utils.cc

@@ -46,17 +46,4 @@ void SetAccountCreationFormsDetectedMessage(
   generation_agent->OnMessageReceived(msg);
 }
 
-void ExpectPasswordGenerationAvailable(
-    TestPasswordGenerationAgent* password_generation,
-    bool available) {
-  if (available) {
-    ASSERT_EQ(1u, password_generation->messages().size());
-    EXPECT_EQ(AutofillHostMsg_ShowPasswordGenerationPopup::ID,
-              password_generation->messages()[0]->type());
-  } else {
-    EXPECT_TRUE(password_generation->messages().empty());
-  }
-  password_generation->clear_messages();
-}
-
 }  // namespace autofill

chrome/renderer/autofill/password_generation_test_utils.h

@@ -20,9 +20,6 @@ void SetNotBlacklistedMessage(TestPasswordGenerationAgent* generation_agent,
 void SetAccountCreationFormsDetectedMessage(TestPasswordGenerationAgent* agent,
                                             blink::WebDocument document,
                                             int form_index);
-void ExpectPasswordGenerationAvailable(TestPasswordGenerationAgent* agent,
-                                       bool available);
-
 }  // namespace autofill
 
 #endif  // CHROME_RENDERER_AUTOFILL_PASSWORD_GENERATION_TEST_UTILS_H_

components/autofill/content/common/autofill_messages.h

@@ -311,6 +311,11 @@ IPC_MESSAGE_ROUTED0(AutofillHostMsg_DidEndTextFieldEditing)
 // Instructs the browser to hide the Autofill popup if it is open.
 IPC_MESSAGE_ROUTED0(AutofillHostMsg_HidePopup)
 
+// Instructs the browser that generation is available for this particular form.
+// This is used for UMA stats.
+IPC_MESSAGE_ROUTED1(AutofillHostMsg_GenerationAvailableForForm,
+                    autofill::PasswordForm)
+
 // Instructs the browser to show the password generation popup at the
 // specified location. This location should be specified in the renderers
 // coordinate system. Form is the form associated with the password field.

components/autofill/content/renderer/password_generation_agent.cc

@@ -329,6 +329,9 @@ void PasswordGenerationAgent::DetermineGenerationElement() {
     password_generation::LogPasswordGenerationEvent(
         password_generation::GENERATION_AVAILABLE);
     possible_account_creation_forms_.clear();
+    Send(new AutofillHostMsg_GenerationAvailableForForm(
+        routing_id(),
+        *generation_form_data_->form));
     return;
   }
 }

components/autofill/content/renderer/test_password_generation_agent.cc

@@ -24,9 +24,4 @@ bool TestPasswordGenerationAgent::ShouldAnalyzeDocument() const {
   return true;
 }
 
-bool TestPasswordGenerationAgent::Send(IPC::Message* message) {
-  messages_.push_back(message);
-  return true;
-}
-
 }  // namespace autofill

components/autofill/content/renderer/test_password_generation_agent.h

@@ -21,21 +21,12 @@ class TestPasswordGenerationAgent : public PasswordGenerationAgent {
 
   // content::RenderFrameObserver implementation:
   bool OnMessageReceived(const IPC::Message& message) override;
-  bool Send(IPC::Message* message) override;
-
-  // Access messages that would have been sent to the browser.
-  const std::vector<IPC::Message*>& messages() const { return messages_.get(); }
-
-  // Clear outgoing message queue.
-  void clear_messages() { messages_.clear(); }
 
   // PasswordGenreationAgent implementation:
   // Always return true to allow loading of data URLs.
   bool ShouldAnalyzeDocument() const override;
 
  private:
-  ScopedVector<IPC::Message> messages_;
-
   DISALLOW_COPY_AND_ASSIGN(TestPasswordGenerationAgent);
 };
 

components/password_manager/core/browser/password_form_manager.cc

@@ -91,6 +91,7 @@ PasswordFormManager::PasswordFormManager(
       is_new_login_(true),
       has_generated_password_(false),
       password_overridden_(false),
+      generation_available_(false),
       password_manager_(password_manager),
       preferred_match_(nullptr),
       is_ignorable_change_password_form_(false),
@@ -106,9 +107,14 @@ PasswordFormManager::PasswordFormManager(
 PasswordFormManager::~PasswordFormManager() {
   UMA_HISTOGRAM_ENUMERATION(
       "PasswordManager.ActionsTakenV3", GetActionsTaken(), kMaxNumActionsTaken);
-  if (has_generated_password_ && submit_result_ == kSubmitResultNotSubmitted)
-    metrics_util::LogPasswordGenerationSubmissionEvent(
-        metrics_util::PASSWORD_NOT_SUBMITTED);
+  if (submit_result_ == kSubmitResultNotSubmitted) {
+    if (has_generated_password_)
+      metrics_util::LogPasswordGenerationSubmissionEvent(
+          metrics_util::PASSWORD_NOT_SUBMITTED);
+    else if (generation_available_)
+      metrics_util::LogPasswordGenerationAvailableSubmissionEvent(
+          metrics_util::PASSWORD_NOT_SUBMITTED);
+  }
   if (form_type_ != kFormTypeUnspecified) {
     UMA_HISTOGRAM_ENUMERATION("PasswordManager.SubmittedFormType", form_type_,
                               kFormTypeMax);
@@ -1004,18 +1010,28 @@ PasswordForm* PasswordFormManager::FindBestMatchForUpdatePassword(
              : best_password_match_it->second;
 }
 
-void PasswordFormManager::SubmitPassed() {
+void PasswordFormManager::LogSubmitPassed() {
+  if (submit_result_ != kSubmitResultFailed) {
+    if (has_generated_password_) {
+      metrics_util::LogPasswordGenerationSubmissionEvent(
+          metrics_util::PASSWORD_SUBMITTED);
+    } else if (generation_available_) {
+      metrics_util::LogPasswordGenerationAvailableSubmissionEvent(
+          metrics_util::PASSWORD_SUBMITTED);
+    }
+  }
   submit_result_ = kSubmitResultPassed;
-  if (has_generated_password_)
-    metrics_util::LogPasswordGenerationSubmissionEvent(
-        metrics_util::PASSWORD_SUBMITTED);
 }
 
-void PasswordFormManager::SubmitFailed() {
-  submit_result_ = kSubmitResultFailed;
-  if (has_generated_password_)
+void PasswordFormManager::LogSubmitFailed() {
+  if (has_generated_password_) {
     metrics_util::LogPasswordGenerationSubmissionEvent(
+        metrics_util::GENERATED_PASSWORD_FORCE_SAVED);
+  } else if (generation_available_) {
+    metrics_util::LogPasswordGenerationAvailableSubmissionEvent(
         metrics_util::PASSWORD_SUBMISSION_FAILED);
+  }
+  submit_result_ = kSubmitResultFailed;
 }
 
 void PasswordFormManager::WipeStoreCopyIfOutdated() {

components/password_manager/core/browser/password_form_manager.h

@@ -155,8 +155,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
 
   // Call these if/when we know the form submission worked or failed.
   // These routines are used to update internal statistics ("ActionsTaken").
-  void SubmitPassed();
-  void SubmitFailed();
+  void LogSubmitPassed();
+  void LogSubmitFailed();
 
   // When attempting to provisionally save |form|, call this to check if it is
   // actually a change-password form which should be ignored, i.e., whether:
@@ -183,6 +183,9 @@ class PasswordFormManager : public PasswordStoreConsumer {
 
   bool password_overridden() const { return password_overridden_; }
 
+  // Called if the user could generate a password for this form.
+  void MarkGenerationAvailable() { generation_available_ = true; }
+
   // Returns the pending credentials.
   const autofill::PasswordForm& pending_credentials() const {
     return pending_credentials_;
@@ -409,6 +412,9 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // Whether the saved password was overridden.
   bool password_overridden_;
 
+  // Whether the user can choose to generate a password for this form.
+  bool generation_available_;
+
   // Set if the user has selected one of the other possible usernames in
   // |pending_credentials_|.
   base::string16 selected_username_;