[Password Generation] Fixes sending votes about the usage of the password generation popup

If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response.

This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote.

BUG=552420

Review-Url: https://codereview.chromium.org/2542093002
Cr-Commit-Position: refs/heads/master@{#439763}

chrome/browser/ui/passwords/manage_passwords_bubble_model.cc

@@ -80,8 +80,8 @@ class ManagePasswordsBubbleModel::InteractionKeeper {
 
   ~InteractionKeeper() = default;
 
-  // Records UMA events and updates the interaction statistics when the bubble
-  // is closed.
+  // Records UMA events, updates the interaction statistics and sends
+  // notifications to the delegate when the bubble is closed.
   void ReportInteractions(const ManagePasswordsBubbleModel* model);
 
   void set_dismissal_reason(
@@ -200,15 +200,22 @@ void ManagePasswordsBubbleModel::InteractionKeeper::ReportInteractions(
 
   if (model->state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE ||
       model->state() == password_manager::ui::PENDING_PASSWORD_STATE) {
+    // Send a notification if there was no interaction with the bubble.
+    bool no_interaction =
+        model->state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE
+            ? update_password_submission_event_ ==
+                  metrics_util::NO_UPDATE_SUBMISSION
+            : dismissal_reason_ == metrics_util::NO_DIRECT_INTERACTION;
+    if (no_interaction && model->delegate_) {
+      model->delegate_->OnNoInteraction();
+    }
+
+    // Send UMA.
     if (update_password_submission_event_ ==
         metrics_util::NO_UPDATE_SUBMISSION) {
       update_password_submission_event_ =
           model->GetUpdateDismissalReason(NO_INTERACTION);
-      if (model->delegate_ &&
-          model->state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE)
-        model->delegate_->OnNoInteractionOnUpdate();
     }
-
     if (update_password_submission_event_ != metrics_util::NO_UPDATE_SUBMISSION)
       LogUpdatePasswordSubmissionEvent(update_password_submission_event_);
   }

chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc

@@ -258,6 +258,7 @@ TEST_F(ManagePasswordsBubbleModelTest, CloseWithoutInteraction) {
   stats.dismissal_count++;
   stats.update_time = now;
   EXPECT_CALL(*GetStore(), AddSiteStatsImpl(stats));
+  EXPECT_CALL(*controller(), OnNoInteraction());
   EXPECT_CALL(*controller(), SavePassword()).Times(0);
   EXPECT_CALL(*controller(), NeverSavePassword()).Times(0);
   DestroyModelExpectReason(
@@ -332,6 +333,7 @@ TEST_F(ManagePasswordsBubbleModelTest, ShowSmartLockWarmWelcome) {
 
   EXPECT_TRUE(model()->ShouldShowGoogleSmartLockWelcome());
   EXPECT_CALL(*GetStore(), AddSiteStatsImpl(_));
+  EXPECT_CALL(*controller(), OnNoInteraction());
   DestroyModel();
   PretendPasswordWaiting();
 
@@ -352,6 +354,7 @@ TEST_F(ManagePasswordsBubbleModelTest, OmitSmartLockWarmWelcome) {
 
   EXPECT_FALSE(model()->ShouldShowGoogleSmartLockWelcome());
   EXPECT_CALL(*GetStore(), AddSiteStatsImpl(_));
+  EXPECT_CALL(*controller(), OnNoInteraction());
   DestroyModel();
   PretendPasswordWaiting();
 

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

@@ -263,16 +263,19 @@ void ManagePasswordsUIController::OnBubbleHidden() {
     UpdateBubbleAndIconVisibility();
 }
 
-void ManagePasswordsUIController::OnNoInteractionOnUpdate() {
-  if (GetState() != password_manager::ui::PENDING_PASSWORD_UPDATE_STATE) {
+void ManagePasswordsUIController::OnNoInteraction() {
+  if (GetState() != password_manager::ui::PENDING_PASSWORD_UPDATE_STATE &&
+      GetState() != password_manager::ui::PENDING_PASSWORD_STATE) {
     // Do nothing if the state was changed. It can happen for example when the
-    // update bubble is active and a page navigation happens.
+    // bubble is active and a page navigation happens.
     return;
   }
+  bool is_update =
+      GetState() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE;
   password_manager::PasswordFormManager* form_manager =
       passwords_data_.form_manager();
   DCHECK(form_manager);
-  form_manager->OnNoInteractionOnUpdate();
+  form_manager->OnNoInteraction(is_update);
 }
 
 void ManagePasswordsUIController::OnNopeUpdateClicked() {
@@ -382,7 +385,7 @@ void ManagePasswordsUIController::NeverSavePasswordInternal() {
   password_manager::PasswordFormManager* form_manager =
       passwords_data_.form_manager();
   DCHECK(form_manager);
-  form_manager->PermanentlyBlacklist();
+  form_manager->OnNeverClicked();
 }
 
 void ManagePasswordsUIController::UpdateBubbleAndIconVisibility() {

chrome/browser/ui/passwords/manage_passwords_ui_controller.h

@@ -94,7 +94,7 @@ class ManagePasswordsUIController
       const override;
   void OnBubbleShown() override;
   void OnBubbleHidden() override;
-  void OnNoInteractionOnUpdate() override;
+  void OnNoInteraction() override;
   void OnNopeUpdateClicked() override;
   void NeverSavePassword() override;
   void SavePassword() override;

chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h

@@ -30,7 +30,7 @@ class ManagePasswordsUIControllerMock : public ManagePasswordsUIController {
                      password_manager::InteractionsStats*());
   MOCK_METHOD0(OnBubbleShown, void());
   MOCK_METHOD0(OnBubbleHidden, void());
-  MOCK_METHOD0(OnNoInteractionOnUpdate, void());
+  MOCK_METHOD0(OnNoInteraction, void());
   MOCK_METHOD0(OnNopeUpdateClicked, void());
   MOCK_METHOD0(NeverSavePassword, void());
   MOCK_METHOD0(SavePassword, void());

chrome/browser/ui/passwords/passwords_model_delegate.h

@@ -60,8 +60,8 @@ class PasswordsModelDelegate {
   // Called from the model when the bubble is hidden.
   virtual void OnBubbleHidden() = 0;
 
-  // Called when the user didn't interact with the Update UI.
-  virtual void OnNoInteractionOnUpdate() = 0;
+  // Called when the user didn't interact with UI.
+  virtual void OnNoInteraction() = 0;
 
   // Called when the user chose not to update password.
   virtual void OnNopeUpdateClicked() = 0;

chrome/browser/ui/passwords/passwords_model_delegate_mock.h

@@ -29,7 +29,7 @@ class PasswordsModelDelegateMock
                      password_manager::InteractionsStats*());
   MOCK_METHOD0(OnBubbleShown, void());
   MOCK_METHOD0(OnBubbleHidden, void());
-  MOCK_METHOD0(OnNoInteractionOnUpdate, void());
+  MOCK_METHOD0(OnNoInteraction, void());
   MOCK_METHOD0(OnNopeUpdateClicked, void());
   MOCK_METHOD0(NeverSavePassword, void());
   MOCK_METHOD0(SavePassword, void());

components/password_manager/core/browser/password_form_manager.h

@@ -25,6 +25,8 @@
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_store.h"
 
+using autofill::FormStructure;
+
 namespace password_manager {
 
 class FormSaver;
@@ -215,8 +217,12 @@ class PasswordFormManager : public FormFetcher::Consumer {
   // Called when the user chose not to update password.
   void OnNopeUpdateClicked();
 
-  // Called when the user didn't interact with Update UI.
-  void OnNoInteractionOnUpdate();
+  // Called when the user clicked "Never" button in the "save password" prompt.
+  void OnNeverClicked();
+
+  // Called when the user didn't interact with UI. |is_update| is true iff
+  // it was the update UI.
+  void OnNoInteraction(bool is_update);
 
   // Saves the outcome of HTML parsing based form classifier to upload proto.
   void SaveGenerationFieldDetectedByClassifier(
@@ -356,36 +362,12 @@ class PasswordFormManager : public FormFetcher::Consumer {
   // UMA.
   int GetActionsTaken() const;
 
-  // Try to label password fields and upload |form_data|. This differs from
-  // AutofillManager::OnFormSubmitted() in a few ways.
-  //   - This function will only label the first <input type="password"> field
-  //     as |password_type|. Other fields will stay unlabeled, as they
-  //     should have been labeled during the upload for OnFormSubmitted().
-  //   - If the |username_field| attribute is nonempty, we will additionally
-  //     label the field with that name as the username field.
-  //   - This function does not assume that |form| is being uploaded during
-  //     the same browsing session as it was originally submitted (as we may
-  //     not have the necessary information to classify the form at that time)
-  //     so it bypasses the cache and doesn't log the same quality UMA metrics.
-  // |login_form_signature| may be empty.  It is non-empty when the user fills
-  // and submits a login form using a generated password. In this case,
-  // |login_form_signature| should be set to the submitted form's signature.
-  // Note that in this case, |form.FormSignature()| gives the signature for the
-  // registration form on which the password was generated, rather than the
-  // submitted form's signature.
-  bool UploadPasswordForm(const autofill::FormData& form_data,
-                          const base::string16& username_field,
+  // Tries to set all votes (e.g. autofill field types, generation vote) to
+  // a |FormStructure| and upload it to the server. Returns true on success.
+  bool UploadPasswordVote(const base::string16& username_field,
                           const autofill::ServerFieldType& password_type,
                           const std::string& login_form_signature);
 
-  // Try to label username, password and new password fields of |observed_form_|
-  // which is considered to be change password forms. Returns true on success.
-  // |password_type| should be equal to NEW_PASSWORD, PROBABLY_NEW_PASSWORD or
-  // NOT_NEW_PASSWORD. These values correspond to cases when the user conrirmed
-  // password update, did nothing or declined to update password respectively.
-  bool UploadChangePasswordForm(const autofill::ServerFieldType& password_type,
-                                const std::string& login_form_signature);
-
   // Adds a vote on password generation usage to |form_structure|.
   void AddGeneratedVote(autofill::FormStructure* form_structure);
 
@@ -435,6 +417,26 @@ class PasswordFormManager : public FormFetcher::Consumer {
   base::Optional<autofill::PasswordForm> UpdatePendingAndGetOldKey(
       std::vector<autofill::PasswordForm>* credentials_to_update);
 
+  // Sets autofill types of username, password and new password fields in
+  // |form_structure|. |password_type| (the autofill type of new password field)
+  // should be equal to NEW_PASSWORD, PROBABLY_NEW_PASSWORD or NOT_NEW_PASSWORD.
+  // These values correspond to cases when the user confirmed password update,
+  // did nothing or declined to update password respectively. The function also
+  // add the types to |available_field_types|.
+  void SetAutofillTypesOnUpdate(
+      const autofill::ServerFieldType password_type,
+      FormStructure* form_structure,
+      autofill::ServerFieldTypeSet* available_field_types);
+
+  // Sets autofill types of username and password fields in |form_structure|.
+  // |username_field| determines the name of the username field. The function
+  // also add the types to |available_field_types|.
+  void SetAutofillTypesOnSave(
+      const base::string16& username_field,
+      const autofill::ServerFieldType password_type,
+      FormStructure* form_structure,
+      autofill::ServerFieldTypeSet* available_field_types);
+
   // Set of nonblacklisted PasswordForms from the DB that best match the form
   // being managed by |this|, indexed by username. They are owned by
   // |form_fetcher_|.

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -62,6 +62,9 @@ namespace password_manager {
 
 namespace {
 
+// Enum that describes what button the user pressed on the save prompt.
+enum SavePromptInteraction { SAVE, NEVER, NO_INTERACTION };
+
 class MockFormSaver : public StubFormSaver {
  public:
   MockFormSaver() = default;
@@ -514,7 +517,7 @@ class PasswordFormManagerTest : public testing::Test {
         form_manager.Update(*saved_match());
         break;
       case autofill::PROBABLY_NEW_PASSWORD:
-        form_manager.OnNoInteractionOnUpdate();
+        form_manager.OnNoInteraction(true /* it is an update */);
         break;
       case autofill::NOT_NEW_PASSWORD:
         form_manager.OnNopeUpdateClicked();
@@ -552,13 +555,17 @@ class PasswordFormManagerTest : public testing::Test {
 
   // The user types username and generates password on SignUp or change password
   // form. The password generation might be triggered automatically or manually.
-  // This function checks that correct vote is uploaded on server.
+  // This function checks that correct vote is uploaded on server. The vote must
+  // be uploaded regardless of the user's interaction with the prompt.
   void GeneratedVoteUploadTest(bool is_manual_generation,
                                bool is_change_password_form,
-                               bool has_generated_password) {
+                               bool has_generated_password,
+                               SavePromptInteraction interaction) {
     SCOPED_TRACE(testing::Message()
                  << "is_manual_generation=" << is_manual_generation
-                 << " is_change_password_form=" << is_change_password_form);
+                 << " is_change_password_form=" << is_change_password_form
+                 << " has_generated_password=" << has_generated_password
+                 << " interaction=" << interaction);
     PasswordForm form(*observed_form());
     form.form_data = saved_match()->form_data;
 
@@ -593,8 +600,11 @@ class PasswordFormManagerTest : public testing::Test {
     fetcher.SetNonFederated(std::vector<const PasswordForm*>(), 0u);
 
     autofill::ServerFieldTypeSet expected_available_field_types;
-    expected_available_field_types.insert(autofill::USERNAME);
-    expected_available_field_types.insert(autofill::PASSWORD);
+    // Don't send autofill votes if the user didn't press "Save" button.
+    if (interaction == SAVE) {
+      expected_available_field_types.insert(autofill::USERNAME);
+      expected_available_field_types.insert(autofill::PASSWORD);
+    }
 
     form_manager.set_is_manual_generation(is_manual_generation);
     base::string16 generation_element = is_change_password_form
@@ -625,7 +635,17 @@ class PasswordFormManagerTest : public testing::Test {
 
     form_manager.ProvisionallySave(
         submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
-    form_manager.Save();
+    switch (interaction) {
+      case SAVE:
+        form_manager.Save();
+        break;
+      case NEVER:
+        form_manager.OnNeverClicked();
+        break;
+      case NO_INTERACTION:
+        form_manager.OnNoInteraction(false /* not an update prompt*/);
+        break;
+    }
   }
 
   PasswordForm* observed_form() { return &observed_form_; }
@@ -2546,17 +2566,21 @@ TEST_F(PasswordFormManagerTest,
   form_manager.Update(*saved_match());
 }
 
+// Checks uploading a vote about the usage of the password generation popup.
 TEST_F(PasswordFormManagerTest, GeneratedVoteUpload) {
-  // Automatic generation, sign-up form.
-  GeneratedVoteUploadTest(false, false, true);
-  // Automatic generation, change password form.
-  GeneratedVoteUploadTest(false, true, true);
-  // Manual generation, sign-up form.
-  GeneratedVoteUploadTest(true, false, true);
-  // Manual generation, change password form.
-  GeneratedVoteUploadTest(true, true, true);
-  // Generation popup was shown, but the user entered its own password.
-  GeneratedVoteUploadTest(true, true, false);
+  bool kFalseTrue[] = {false, true};
+  SavePromptInteraction kSavePromptInterations[] = {SAVE, NEVER,
+                                                    NO_INTERACTION};
+  for (bool is_manual_generation : kFalseTrue) {
+    for (bool is_change_password_form : kFalseTrue) {
+      for (bool has_generated_password : kFalseTrue) {
+        for (SavePromptInteraction interaction : kSavePromptInterations) {
+          GeneratedVoteUploadTest(is_manual_generation, is_change_password_form,
+                                  has_generated_password, interaction);
+        }
+      }
+    }
+  }
 }
 
 TEST_F(PasswordFormManagerTest, FormClassifierVoteUpload) {