[Password Generation] Some changes of password generation metrics

- introduces the metric of whether manual or automatic generation was triggered
- fixes a bug in reporting NO_SIGN_UP_DETECTED and SIGN_UP_DETECTED
- disables reporting of PasswordGenerationEvent if generation is disabled

Bug: 851401
Change-Id: I4ccd4bfb5847d108a671571601e85fbf876de637
Reviewed-on: https://chromium-review.googlesource.com/1095177Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566794}

components/autofill/content/renderer/password_generation_agent.cc

@@ -214,6 +214,30 @@ void PasswordGenerationAgent::DidCommitProvisionalLoad(
 void PasswordGenerationAgent::DidFinishDocumentLoad() {
   // Update stats for main frame navigation.
   if (!render_frame()->GetWebFrame()->Parent()) {
+    // Log statistics after navigation so that we only log once per page.
+    if (enabled_ || generation_form_data_) {
+      if (generation_form_data_ &&
+          !generation_form_data_->password_elements.empty()) {
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::SIGN_UP_DETECTED);
+      } else {
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::NO_SIGN_UP_DETECTED);
+      }
+      if (password_edited_) {
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::PASSWORD_EDITED);
+      }
+      if (generation_popup_shown_) {
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::GENERATION_POPUP_SHOWN);
+      }
+      if (editing_popup_shown_) {
+        password_generation::LogPasswordGenerationEvent(
+            password_generation::EDITING_POPUP_SHOWN);
+      }
+    }
+
     // In every navigation, the IPC message sent by the password autofill
     // manager to query whether the current form is blacklisted or not happens
     // when the document load finishes, so we need to clear previous states
@@ -226,34 +250,10 @@ void PasswordGenerationAgent::DidFinishDocumentLoad() {
     generation_enabled_forms_.clear();
     generation_element_.Reset();
     possible_account_creation_forms_.clear();
-
-    // Log statistics after navigation so that we only log once per page.
-    if (generation_form_data_ &&
-        generation_form_data_->password_elements.empty()) {
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::NO_SIGN_UP_DETECTED);
-    } else {
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::SIGN_UP_DETECTED);
-    }
     generation_form_data_.reset();
     password_is_generated_ = false;
-    if (password_edited_) {
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::PASSWORD_EDITED);
-    }
     password_edited_ = false;
-
-    if (generation_popup_shown_) {
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::GENERATION_POPUP_SHOWN);
-    }
     generation_popup_shown_ = false;
-
-    if (editing_popup_shown_) {
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::EDITING_POPUP_SHOWN);
-    }
     editing_popup_shown_ = false;
   }
 
@@ -623,32 +623,13 @@ bool PasswordGenerationAgent::TextDidChangeInTextField(
   return true;
 }
 
-void PasswordGenerationAgent::AutomaticGenerationStatusChanged(bool available) {
-  if (available) {
-    if (!render_frame() || generation_element_.IsNull())
-      return;
-    LogMessage(
-        Logger::STRING_GENERATION_RENDERER_AUTOMATIC_GENERATION_AVAILABLE);
-    autofill::password_generation::PasswordGenerationUIData
-        password_generation_ui_data(
-            render_frame()->GetRenderView()->ElementBoundsInWindow(
-                generation_element_),
-            generation_element_.MaxLength(),
-            generation_element_.NameForAutofill().Utf16(),
-            *generation_form_data_->form);
-    GetPasswordManagerClient()->AutomaticGenerationStatusChanged(
-        true, password_generation_ui_data);
-    generation_popup_shown_ = true;
-  } else {
-    GetPasswordManagerClient()->AutomaticGenerationStatusChanged(false,
-                                                                 base::nullopt);
-  }
-}
-
-void PasswordGenerationAgent::ShowManualGenerationPopup() {
+void PasswordGenerationAgent::ShowGenerationPopup(bool is_manual_generation) {
   if (!render_frame() || generation_element_.IsNull())
     return;
-  LogMessage(Logger::STRING_GENERATION_RENDERER_SHOW_MANUAL_GENERATION_POPUP);
+  LogMessage(
+      is_manual_generation
+          ? Logger::STRING_GENERATION_RENDERER_SHOW_MANUAL_GENERATION_POPUP
+          : Logger::STRING_GENERATION_RENDERER_AUTOMATIC_GENERATION_AVAILABLE);
   autofill::password_generation::PasswordGenerationUIData
       password_generation_ui_data(
           render_frame()->GetRenderView()->ElementBoundsInWindow(
@@ -656,9 +637,24 @@ void PasswordGenerationAgent::ShowManualGenerationPopup() {
           generation_element_.MaxLength(),
           generation_element_.NameForAutofill().Utf16(),
           *generation_form_data_->form);
-  GetPasswordManagerClient()->ShowManualPasswordGenerationPopup(
-      password_generation_ui_data);
   generation_popup_shown_ = true;
+  if (is_manual_generation) {
+    GetPasswordManagerClient()->ShowManualPasswordGenerationPopup(
+        password_generation_ui_data);
+  } else {
+    GetPasswordManagerClient()->AutomaticGenerationStatusChanged(
+        true, password_generation_ui_data);
+  }
+}
+
+void PasswordGenerationAgent::AutomaticGenerationStatusChanged(bool available) {
+  if (available) {
+    ShowGenerationPopup(false /* is_manual_generation */);
+  } else {
+    // Hide the generation popup.
+    GetPasswordManagerClient()->AutomaticGenerationStatusChanged(false,
+                                                                 base::nullopt);
+  }
 }
 
 void PasswordGenerationAgent::ShowEditingPopup() {
@@ -695,7 +691,7 @@ void PasswordGenerationAgent::PasswordNoLongerGenerated() {
 
 void PasswordGenerationAgent::UserTriggeredGeneratePassword() {
   if (SetUpUserTriggeredGeneration())
-    ShowManualGenerationPopup();
+    ShowGenerationPopup(true /* is_manual_generation */);
 }
 
 void PasswordGenerationAgent::UserSelectedManualGenerationOption() {
@@ -703,7 +699,7 @@ void PasswordGenerationAgent::UserSelectedManualGenerationOption() {
     last_focused_password_element_.SetAutofillValue(blink::WebString());
     last_focused_password_element_.SetAutofillState(
         WebAutofillState::kNotFilled);
-    ShowManualGenerationPopup();
+    ShowGenerationPopup(true /* is_manual_generation */);
   }
 }
 

components/autofill/content/renderer/password_generation_agent.h

@@ -118,15 +118,14 @@ class PasswordGenerationAgent : public content::RenderFrameObserver,
   // all required information is collected.
   bool SetUpUserTriggeredGeneration();
 
+  // Shows a generation popup.
+  void ShowGenerationPopup(bool is_manual_generation);
+
   // Signals the browser that it should offer or rescind automatic password
   // generation depending whether the user has just focused a form field
   // suitable for generation or has changed focus from such a field.
   void AutomaticGenerationStatusChanged(bool available);
 
-  // Show password generation UI anchored at |generation_element_|. This is
-  // only called for manual password generation.
-  void ShowManualGenerationPopup();
-
   // Show UI for editing a generated password at |generation_element_|.
   void ShowEditingPopup();
 

components/autofill/core/common/password_generation_util.h

@@ -62,8 +62,7 @@ enum PasswordGenerationEvent {
   // User focused the password field containing the generated password.
   EDITING_POPUP_SHOWN,
 
-  // Generation enabled because autocomplete attributes for username and
-  // new-password are set.
+  // Generation enabled because autocomplete attributes for new-password is set.
   AUTOCOMPLETE_ATTRIBUTES_ENABLED_GENERATION,
 
   // Generation is triggered by the user from the context menu.

components/password_manager/core/browser/password_form_manager.cc

@@ -310,6 +310,13 @@ void PasswordFormManager::Save() {
       DidPreferenceChange(best_matches_, pending_credentials_.username_value)) {
     SetUserAction(UserAction::kChoose);
   }
+  if (user_action_ == UserAction::kOverridePassword &&
+      pending_credentials_.type == PasswordForm::TYPE_GENERATED &&
+      !has_generated_password_) {
+    metrics_util::LogPasswordGenerationSubmissionEvent(
+        metrics_util::PASSWORD_OVERRIDDEN);
+    pending_credentials_.type = PasswordForm::TYPE_MANUAL;
+  }
 
   if (is_new_login_) {
     SanitizePossibleUsernames(&pending_credentials_);
@@ -328,7 +335,7 @@ void PasswordFormManager::Save() {
   }
 
   // This is not in ProcessUpdate() to catch PSL matched credentials.
-  if (pending_credentials_.times_used != 0 &&
+  if (pending_credentials_.times_used == 1 &&
       pending_credentials_.type == PasswordForm::TYPE_GENERATED) {
     metrics_util::LogPasswordGenerationSubmissionEvent(
         metrics_util::PASSWORD_USED);
@@ -770,14 +777,6 @@ void PasswordFormManager::CreatePendingCredentials() {
     pending_credentials_.signon_realm = submitted_form_->signon_realm;
   }
 
-  if (user_action_ == UserAction::kOverridePassword &&
-      pending_credentials_.type == PasswordForm::TYPE_GENERATED &&
-      !has_generated_password_) {
-    metrics_util::LogPasswordGenerationSubmissionEvent(
-        metrics_util::PASSWORD_OVERRIDDEN);
-    pending_credentials_.type = PasswordForm::TYPE_MANUAL;
-  }
-
   if (has_generated_password_)
     pending_credentials_.type = PasswordForm::TYPE_GENERATED;
 }

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -713,6 +713,9 @@ class PasswordFormManagerTest : public testing::Test {
       histogram_tester.ExpectUniqueSample(
           "PasswordGeneration.GeneratedPasswordWasEdited",
           generated_password_changed /* sample */, 1);
+      histogram_tester.ExpectUniqueSample(
+          "PasswordGeneration.IsTriggeredManually",
+          is_manual_generation /* sample */, 1);
     }
     Mock::VerifyAndClearExpectations(
         client()->mock_driver()->mock_autofill_download_manager());
@@ -2653,6 +2656,52 @@ TEST_F(PasswordFormManagerTest, GenerationStatusNotUpdatedIfPasswordUnchanged) {
   EXPECT_EQ(PasswordForm::TYPE_GENERATED, new_credentials.type);
   histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
                                      metrics_util::PASSWORD_USED, 1);
+
+  // On the second reuse, the metric is not reported.
+  generated_form.times_used = 1;
+  fake_form_fetcher()->SetNonFederated({&generated_form}, 0u);
+  form_manager()->ProvisionallySave(submitted_form);
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, _));
+  form_manager()->Save();
+  histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
+                                     metrics_util::PASSWORD_USED, 1);
+}
+
+TEST_F(PasswordFormManagerTest, GeneratedPasswordIsOverridden) {
+  base::HistogramTester histogram_tester;
+
+  PasswordForm generated_form = *observed_form();
+  generated_form.type = PasswordForm::TYPE_GENERATED;
+  generated_form.username_value = ASCIIToUTF16("username");
+  generated_form.password_value = ASCIIToUTF16("password2");
+  generated_form.preferred = true;
+
+  PasswordForm submitted_form(generated_form);
+  submitted_form.password_value = ASCIIToUTF16("another_password");
+
+  fake_form_fetcher()->SetNonFederated({&generated_form}, 0u);
+
+  form_manager()->ProvisionallySave(submitted_form);
+
+  PasswordForm new_credentials;
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, nullptr))
+      .WillOnce(testing::SaveArg<0>(&new_credentials));
+  form_manager()->Save();
+
+  EXPECT_EQ(PasswordForm::TYPE_MANUAL, new_credentials.type);
+  EXPECT_EQ(ASCIIToUTF16("another_password"), new_credentials.password_value);
+  histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
+                                     metrics_util::PASSWORD_OVERRIDDEN, 1);
+
+  // On the reuse of the overriden password, the metric is not reported.
+  fake_form_fetcher()->SetNonFederated({&new_credentials}, 0u);
+  form_manager()->ProvisionallySave(new_credentials);
+  EXPECT_CALL(MockFormSaver::Get(form_manager()), Update(_, _, _, _));
+  form_manager()->Save();
+  histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
+                                     metrics_util::PASSWORD_OVERRIDDEN, 1);
+  histogram_tester.ExpectBucketCount("PasswordGeneration.SubmissionEvent",
+                                     metrics_util::PASSWORD_USED, 0);
 }
 
 // Test that ProcessFrame is called on receiving matches from the fetcher,

components/password_manager/core/browser/votes_uploader.cc

@@ -375,6 +375,8 @@ void VotesUploader::AddGeneratedVote(FormStructure* form_structure) {
   AutofillUploadContents::Field::PasswordGenerationType type =
       AutofillUploadContents::Field::NO_GENERATION;
   if (has_generated_password_) {
+    UMA_HISTOGRAM_BOOLEAN("PasswordGeneration.IsTriggeredManually",
+                          is_manual_generation_);
     if (is_manual_generation_) {
       type = is_possible_change_password_form_
                  ? AutofillUploadContents::Field::

tools/metrics/histograms/enums.xml

@@ -4094,6 +4094,11 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="1" label="Invalid"/>
 </enum>
 
+<enum name="BooleanIsGenerationTriggeredManually">
+  <int value="0" label="Password generation was triggered automatically"/>
+  <int value="1" label="A user triggered password generation"/>
+</enum>
+
 <enum name="BooleanIsLastSharedAppInfoRetrieved">
   <int value="0" label="Not retrieved"/>
   <int value="1" label="Retrieved"/>

tools/metrics/histograms/histograms.xml

@@ -66698,6 +66698,14 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="PasswordGeneration.IsTriggeredManually"
+    enum="BooleanIsGenerationTriggeredManually">
+  <owner>kolos@chromium.org</owner>
+  <summary>
+    Measures the frequency of manually triggered password generations.
+  </summary>
+</histogram>
+
 <histogram name="PasswordGeneration.SubmissionAvailableEvent"
     enum="PasswordSubmissionEvent">
   <owner>gcasto@chromium.org</owner>