[Passwords] Compute User Actions in PasswordFormMetricsRecorder

This change replaces PasswordFormMetricsRecorder's SetUserAction with a
ComputeUserAction API. This allows for removing duplicated code between
the old and new PasswordFormManager and fixes issues with metrics
recording, when both versions of the form manager are active at the same
time.

Bug: 926902
Change-Id: I61373e88d8349fb2e52a5d89ca7b4ec6eda7a305
Reviewed-on: https://chromium-review.googlesource.com/c/1448188
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#628357}(cherry picked from commit bd56e6922f2f6a7591e68c06d1f3f6470dfb046c)
Reviewed-on: https://chromium-review.googlesource.com/c/1451923
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/branch-heads/3683@{#159}
Cr-Branched-From: e51029943e0a38dd794b73caaf6373d5496ae783-refs/heads/master@{#625896}
diff --git a/components/password_manager/core/browser/new_password_form_manager.cc b/components/password_manager/core/browser/new_password_form_manager.cc
index 3e8a5ab..21f7c7e 100644
--- a/components/password_manager/core/browser/new_password_form_manager.cc
+++ b/components/password_manager/core/browser/new_password_form_manager.cc
@@ -16,6 +16,7 @@
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
 #include "components/password_manager/core/browser/form_fetcher_impl.h"
 #include "components/password_manager/core/browser/form_saver.h"
+#include "components/password_manager/core/browser/password_form_filling.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
@@ -627,10 +628,10 @@
   // TODO(https://crbug.com/831123). Implement correct treating of federated
   // matches.
   std::vector<const PasswordForm*> federated_matches;
-  likely_form_filling_ = SendFillInformationToRenderer(
-      *client_, driver_.get(), IsBlacklisted(), *observed_password_form.get(),
-      best_matches_, federated_matches, preferred_match_,
-      metrics_recorder_.get());
+  SendFillInformationToRenderer(*client_, driver_.get(), IsBlacklisted(),
+                                *observed_password_form.get(), best_matches_,
+                                federated_matches, preferred_match_,
+                                metrics_recorder_.get());
 }
 
 void NewPasswordFormManager::FillForm(const FormData& observed_form) {
@@ -716,6 +717,11 @@
   if (!parsed_submitted_form_)
     return;
 
+  // Calculate the user's action based on existing matches and the submitted
+  // form.
+  metrics_recorder_->CalculateUserAction(best_matches_,
+                                         *parsed_submitted_form_);
+
   // This function might be called multiple times so set variables that are
   // changed in this function to initial states.
   is_new_login_ = true;
@@ -739,9 +745,6 @@
       // from Android apps, store a copy with the current origin and signon
       // realm. This ensures that on the next visit, a precise match is found.
       is_new_login_ = true;
-      metrics_recorder_->SetUserAction(password_overridden_
-                                           ? UserAction::kOverridePassword
-                                           : UserAction::kChoosePslMatch);
 
       // Update credential to reflect that it has been used for submission.
       // If this isn't updated, then password generation uploads are off for
@@ -780,19 +783,6 @@
       }
     } else {  // Not a PSL match but a match of an already stored credential.
       is_new_login_ = false;
-      if (password_overridden_) {
-        metrics_recorder_->SetUserAction(UserAction::kOverridePassword);
-      } else {
-        // In case |saved_form| is pointing to the same form as
-        // |preferred_match_| and the form was filled on page load, the user
-        // either did not do anything, or re-selected the default option.
-        // Otherwise, the user purposefully chose a credential.
-        metrics_recorder_->SetUserAction(
-            saved_form == preferred_match_ &&
-                    likely_form_filling_ == LikelyFormFilling::kFillOnPageLoad
-                ? UserAction::kNone
-                : UserAction::kChoose);
-      }
     }
   } else if (!best_matches_.empty() &&
              parsed_submitted_form_->type != PasswordForm::TYPE_API &&
@@ -943,8 +933,6 @@
 void NewPasswordFormManager::CreatePendingCredentialsForNewCredentials(
     const PasswordForm& submitted_password_form,
     const base::string16& password_element) {
-  // User typed in a new, unknown username.
-  metrics_recorder_->SetUserAction(UserAction::kOverrideUsernameAndPassword);
   // TODO(https://crbug.com/831123): Replace parsing of the observed form with
   // usage of already parsed submitted form.
   std::unique_ptr<PasswordForm> parsed_observed_form =
diff --git a/components/password_manager/core/browser/new_password_form_manager.h b/components/password_manager/core/browser/new_password_form_manager.h
index 248192e..82306a2 100644
--- a/components/password_manager/core/browser/new_password_form_manager.h
+++ b/components/password_manager/core/browser/new_password_form_manager.h
@@ -18,7 +18,6 @@
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/form_parsing/form_parser.h"
 #include "components/password_manager/core/browser/form_parsing/password_field_prediction.h"
-#include "components/password_manager/core/browser/password_form_filling.h"
 #include "components/password_manager/core/browser/password_form_manager_for_ui.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #include "components/password_manager/core/browser/password_form_user_action.h"
@@ -288,9 +287,6 @@
 
   VotesUploader votes_uploader_;
 
-  // Probable filling mechanism used in the renderer for this password form.
-  LikelyFormFilling likely_form_filling_ = LikelyFormFilling::kNoFilling;
-
   // |is_submitted_| = true means that a submission of the managed form was seen
   // and then |submitted_form_| contains the submitted form.
   bool is_submitted_ = false;
diff --git a/components/password_manager/core/browser/new_password_form_manager_unittest.cc b/components/password_manager/core/browser/new_password_form_manager_unittest.cc
index dd0e2146..f67907b 100644
--- a/components/password_manager/core/browser/new_password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/new_password_form_manager_unittest.cc
@@ -742,7 +742,7 @@
   EXPECT_TRUE(
       form_manager_->ProvisionallySaveIfIsManaged(submitted_form, &driver_));
   CheckPendingCredentials(expected, form_manager_->GetPendingCredentials());
-  EXPECT_EQ(UserAction::kNone,
+  EXPECT_EQ(UserAction::kOverridePassword,
             form_manager_->GetMetricsRecorder()->GetUserAction());
 }
 
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 4a8b29e..0458d40 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -27,6 +27,7 @@
 #include "components/password_manager/core/browser/form_fetcher_impl.h"
 #include "components/password_manager/core/browser/form_saver.h"
 #include "components/password_manager/core/browser/log_manager.h"
+#include "components/password_manager/core/browser/password_form_filling.h"
 #include "components/password_manager/core/browser/password_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
@@ -557,10 +558,10 @@
     return;
   if (!driver)
     return;
-  likely_form_filling_ = SendFillInformationToRenderer(
-      *client_, driver.get(), IsBlacklisted(), observed_form_, best_matches_,
-      form_fetcher_->GetFederatedMatches(), preferred_match_,
-      GetMetricsRecorder());
+  SendFillInformationToRenderer(*client_, driver.get(), IsBlacklisted(),
+                                observed_form_, best_matches_,
+                                form_fetcher_->GetFederatedMatches(),
+                                preferred_match_, GetMetricsRecorder());
 }
 
 void PasswordFormManager::ProcessLoginPrompt() {
@@ -618,6 +619,10 @@
   DCHECK(submitted_form_);
   ValueElementPair password_to_save(PasswordToSave(*submitted_form_));
 
+  // Calculate the user's action based on existing matches and the submitted
+  // form.
+  metrics_recorder_->CalculateUserAction(best_matches_, *submitted_form_);
+
   // Look for the actually submitted credentials in the list of previously saved
   // credentials that were available to autofilling.
   const PasswordForm* saved_form = FindBestSavedMatch(submitted_form_.get());
@@ -632,9 +637,6 @@
       // from Android apps, store a copy with the current origin and signon
       // realm. This ensures that on the next visit, a precise match is found.
       is_new_login_ = true;
-      metrics_recorder_->SetUserAction(password_overridden_
-                                           ? UserAction::kOverridePassword
-                                           : UserAction::kChoosePslMatch);
 
       // Update credential to reflect that it has been used for submission.
       // If this isn't updated, then password generation uploads are off for
@@ -673,21 +675,6 @@
       }
     } else {  // Not a PSL match but a match of an already stored credential.
       is_new_login_ = false;
-      if (password_overridden_) {
-        // Stored credential matched by username but with mismatching password.
-        // This means the user has overridden the password.
-        metrics_recorder_->SetUserAction(UserAction::kOverridePassword);
-      } else {
-        // In case |saved_form| is pointing to the same form as
-        // |preferred_match_| and the form was filled on page load, the user
-        // either did not do anything, or re-selected the default option.
-        // Otherwise, the user purposefully chose a credential.
-        metrics_recorder_->SetUserAction(
-            saved_form == preferred_match_ &&
-                    likely_form_filling_ == LikelyFormFilling::kFillOnPageLoad
-                ? UserAction::kNone
-                : UserAction::kChoose);
-      }
     }
   } else if (!best_matches_.empty() &&
              submitted_form_->type != autofill::PasswordForm::TYPE_API &&
@@ -834,8 +821,6 @@
 
 void PasswordFormManager::CreatePendingCredentialsForNewCredentials(
     const base::string16& password_element) {
-  // User typed in a new, unknown username.
-  metrics_recorder_->SetUserAction(UserAction::kOverrideUsernameAndPassword);
   pending_credentials_ = observed_form_;
   pending_credentials_.username_element = submitted_form_->username_element;
   pending_credentials_.username_value = submitted_form_->username_value;
diff --git a/components/password_manager/core/browser/password_form_manager.h b/components/password_manager/core/browser/password_form_manager.h
index a1f422a..5f80bff 100644
--- a/components/password_manager/core/browser/password_form_manager.h
+++ b/components/password_manager/core/browser/password_form_manager.h
@@ -22,7 +22,6 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/signatures_util.h"
 #include "components/password_manager/core/browser/form_fetcher.h"
-#include "components/password_manager/core/browser/password_form_filling.h"
 #include "components/password_manager/core/browser/password_form_manager_for_ui.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #include "components/password_manager/core/browser/password_form_user_action.h"
@@ -401,9 +400,6 @@
 
   VotesUploader votes_uploader_;
 
-  // Probable filling mechanism used in the renderer for this password form.
-  LikelyFormFilling likely_form_filling_ = LikelyFormFilling::kNoFilling;
-
   // Takes care of recording metrics and events for this PasswordFormManager.
   // Make sure to call Init before using |*this|, to ensure it is not null.
   scoped_refptr<PasswordFormMetricsRecorder> metrics_recorder_;
diff --git a/components/password_manager/core/browser/password_form_metrics_recorder.cc b/components/password_manager/core/browser/password_form_metrics_recorder.cc
index 8261074..51910f6 100644
--- a/components/password_manager/core/browser/password_form_metrics_recorder.cc
+++ b/components/password_manager/core/browser/password_form_metrics_recorder.cc
@@ -287,7 +287,67 @@
   manager_action_ = manager_action;
 }
 
-void PasswordFormMetricsRecorder::SetUserAction(UserAction user_action) {
+void PasswordFormMetricsRecorder::CalculateUserAction(
+    const std::map<base::string16, const autofill::PasswordForm*>& best_matches,
+    const autofill::PasswordForm& submitted_form) {
+  const base::string16& submitted_password =
+      !submitted_form.new_password_value.empty()
+          ? submitted_form.new_password_value
+          : submitted_form.password_value;
+
+  if (submitted_form.username_value.empty()) {
+    // In case the submitted form does not have a username field we do not
+    // autofill. Thus the user either explicitly chose this credential from the
+    // dropdown, or created a new password.
+    for (const auto& match : best_matches) {
+      if (match.second->password_value == submitted_password) {
+        user_action_ = UserAction::kChoose;
+        return;
+      }
+    }
+
+    user_action_ = UserAction::kOverridePassword;
+    return;
+  }
+
+  // In case the submitted form has a username value, check if there is an
+  // existing match with the same username. If not, the user created a new
+  // credential.
+  auto found = best_matches.find(submitted_form.username_value);
+  if (found == best_matches.end()) {
+    user_action_ = UserAction::kOverrideUsernameAndPassword;
+    return;
+  }
+
+  // Otherwise check if the user changed the password.
+  const autofill::PasswordForm* existing_match = found->second;
+  if (existing_match->password_value != submitted_password) {
+    user_action_ = UserAction::kOverridePassword;
+    return;
+  }
+
+  // If the existing match is a PSL match, the user purposefully chose it, since
+  // PSL credentials are not autofilled.
+  if (existing_match->is_public_suffix_match) {
+    user_action_ = UserAction::kChoosePslMatch;
+    return;
+  }
+
+  // Lastly, in case the existing match is not a preferred match, or the form
+  // was not filled on page load, the user purposefully chose a credential.
+  // Otherwise the user either did not do anything, or re-selected the default
+  // option.
+  if (!existing_match->preferred ||
+      manager_action_ != kManagerActionAutofilled) {
+    user_action_ = UserAction::kChoose;
+    return;
+  }
+
+  user_action_ = UserAction::kNone;
+}
+
+void PasswordFormMetricsRecorder::SetUserActionForTesting(
+    UserAction user_action) {
   user_action_ = user_action;
 }
 
diff --git a/components/password_manager/core/browser/password_form_metrics_recorder.h b/components/password_manager/core/browser/password_form_metrics_recorder.h
index 265a010..c406d44 100644
--- a/components/password_manager/core/browser/password_form_metrics_recorder.h
+++ b/components/password_manager/core/browser/password_form_metrics_recorder.h
@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <map>
 #include <memory>
 #include <set>
 #include <vector>
@@ -307,10 +308,20 @@
   // is was overridden for the form.
   void ReportSpecPriorityForGeneratedPassword(uint32_t spec_priority);
 
-  // Stores the password manager and user actions. During destruction the last
-  // set values will be logged.
+  // Stores the password manager action. During destruction the last
+  // set value will be logged.
   void SetManagerAction(ManagerAction manager_action);
-  void SetUserAction(UserAction user_action);
+
+  // Calculates the user's action depending on the submitted form and existing
+  // matches. Also inspects |manager_action_| to correctly detect if the
+  // user chose a credential.
+  void CalculateUserAction(
+      const std::map<base::string16, const autofill::PasswordForm*>&
+          best_matches,
+      const autofill::PasswordForm& submitted_form);
+
+  // Allow tests to explicitly set a value for |user_action_|.
+  void SetUserActionForTesting(UserAction user_action);
 
   // Gets the current value of |user_action_|.
   UserAction GetUserAction() const;
diff --git a/components/password_manager/core/browser/password_form_metrics_recorder_unittest.cc b/components/password_manager/core/browser/password_form_metrics_recorder_unittest.cc
index 8d31f79..2f125e1 100644
--- a/components/password_manager/core/browser/password_form_metrics_recorder_unittest.cc
+++ b/components/password_manager/core/browser/password_form_metrics_recorder_unittest.cc
@@ -269,8 +269,7 @@
           test.is_main_frame_secure, &test_ukm_recorder);
 
       recorder->SetManagerAction(test.manager_action);
-      if (test.user_action != UserAction::kNone)
-        recorder->SetUserAction(test.user_action);
+      recorder->SetUserActionForTesting(test.user_action);
       if (test.submit_result ==
           PasswordFormMetricsRecorder::kSubmitResultFailed) {
         recorder->LogSubmitFailed();
@@ -297,30 +296,6 @@
   }
 }
 
-// Test that in the case of a sequence of user actions, only the last one is
-// recorded in ActionsV3 but all are recorded as UMA user actions.
-TEST(PasswordFormMetricsRecorder, ActionSequence) {
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
-  ukm::TestAutoSetUkmRecorder test_ukm_recorder;
-  base::HistogramTester histogram_tester;
-  base::UserActionTester user_action_tester;
-
-  // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
-  // on destruction.
-  {
-    auto recorder = CreatePasswordFormMetricsRecorder(
-        true /*is_main_frame_secure*/, &test_ukm_recorder);
-    recorder->SetManagerAction(
-        PasswordFormMetricsRecorder::kManagerActionAutofilled);
-    recorder->SetUserAction(UserAction::kChoosePslMatch);
-    recorder->SetUserAction(UserAction::kOverrideUsernameAndPassword);
-    recorder->LogSubmitPassed();
-  }
-
-  EXPECT_THAT(histogram_tester.GetAllSamples("PasswordManager.ActionsTakenV3"),
-              ::testing::ElementsAre(base::Bucket(39, 1)));
-}
-
 TEST(PasswordFormMetricsRecorder, SubmittedFormType) {
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   static constexpr struct {