Add UKM for comparing password form parsing for saving

When the user submits a form and Chrome understands that as a
successful submission, it parses the submitted data into a
PasswordForm ready to be saved into the PasswordStore.

Currently, two codepaths exists to do so. The old one involves a
PasswordFormManager and an old FormData parser, the new one involves a
NewPasswordFormManager and a new FormData parser.

This CL adds a metric comparing the result of both codepaths.

The privacy review of password manager UKM metrics was done on
https://crbug.com/728707.

Bug: 845426
Change-Id: Ie8c11d4ff6e0a17029ec05104a560fe764597f1e
Reviewed-on: https://chromium-review.googlesource.com/1243126
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595312}

components/password_manager/core/browser/password_form_metrics_recorder.cc

@@ -283,6 +283,11 @@ void PasswordFormMetricsRecorder::RecordParsingsComparisonResult(
       static_cast<uint64_t>(comparison_result));
 }
 
+void PasswordFormMetricsRecorder::RecordParsingOnSavingDifference(
+    uint64_t comparison_result) {
+  ukm_entry_builder_.SetParsingOnSavingDifference(comparison_result);
+}
+
 void PasswordFormMetricsRecorder::RecordShowManualFallbackForSaving(
     bool has_generated_password,
     bool is_update) {

components/password_manager/core/browser/password_form_metrics_recorder.h

@@ -158,7 +158,7 @@ class PasswordFormMetricsRecorder
     kCorrectedUsernameInForm = 200,
   };
 
-  // Old and new form parsings comparison result.
+  // Result of comparing of the old and new form parsing for filling.
   enum class ParsingComparisonResult {
     kSame,
     kDifferent,
@@ -168,6 +168,21 @@ class PasswordFormMetricsRecorder
     kMax
   };
 
+  // Result of comparing of the old and new form parsing for saving. Multiple
+  // values are meant to be combined and reported in a single number as a
+  // bitmask.
+  enum class ParsingOnSavingDifference {
+    // Different fields were identified for username or password.
+    kFields = 1 << 0,
+    // Signon_realms are different.
+    kSignonRealm = 1 << 1,
+    // One password form manager wants to update, the other to save as new.
+    kNewLoginStatus = 1 << 2,
+    // One password form manager thinks the password is generated, the other
+    // does not.
+    kGenerated = 1 << 3,
+  };
+
   // Indicator whether the user has seen a password generation popup and why.
   enum class PasswordGenerationPopupShown {
     kNotShown = 0,
@@ -272,6 +287,10 @@ class PasswordFormMetricsRecorder
   void RecordParsingsComparisonResult(
       ParsingComparisonResult comparison_result);
 
+  // Records the comparison of the old and new password form parsing for saving.
+  // |comparison_result| is a bitmask of values from ParsingOnSavingDifference.
+  void RecordParsingOnSavingDifference(uint64_t comparison_result);
+
   // Records that Chrome noticed that it should show a manual fallback for
   // saving.
   void RecordShowManualFallbackForSaving(bool has_generated_password,

components/password_manager/core/browser/password_manager.cc

@@ -324,6 +324,40 @@ FindAndCloneMatchedNewPasswordFormManager(
   return nullptr;
 }
 
+// Records the difference between how |old_manager| and |new_manager| understood
+// the pending credentials.
+void RecordParsingOnSavingDifference(
+    const PasswordFormManagerInterface& old_manager,
+    const PasswordFormManagerInterface& new_manager,
+    PasswordFormMetricsRecorder* metrics_recorder) {
+  const PasswordForm& old_form = old_manager.GetPendingCredentials();
+  const PasswordForm& new_form = new_manager.GetPendingCredentials();
+  uint64_t result = 0;
+
+  if (old_form.username_element != new_form.username_element ||
+      old_form.username_value != new_form.username_value ||
+      old_form.password_element != new_form.password_element ||
+      old_form.password_value != new_form.password_value) {
+    result |= static_cast<int>(
+        PasswordFormMetricsRecorder::ParsingOnSavingDifference::kFields);
+  }
+  if (old_form.signon_realm != new_form.signon_realm) {
+    result |= static_cast<int>(
+        PasswordFormMetricsRecorder::ParsingOnSavingDifference::kSignonRealm);
+  }
+  if (old_manager.IsNewLogin() != new_manager.IsNewLogin()) {
+    result |= static_cast<int>(PasswordFormMetricsRecorder::
+                                   ParsingOnSavingDifference::kNewLoginStatus);
+  }
+  if (old_manager.HasGeneratedPassword() !=
+      new_manager.HasGeneratedPassword()) {
+    result |= static_cast<int>(
+        PasswordFormMetricsRecorder::ParsingOnSavingDifference::kGenerated);
+  }
+
+  metrics_recorder->RecordParsingOnSavingDifference(result);
+}
+
 }  // namespace
 
 // static
@@ -1067,6 +1101,18 @@ void PasswordManager::OnLoginSuccessful() {
     return;
   }
 
+  // TODO(https://crbug.com/831123): Remove logging when the old form parsing is
+  // removed.
+  if (is_new_form_parsing_for_saving_enabled_) {
+    // In this case, |submitted_manager| points to a NewPasswordFormManager and
+    // |provisional_save_manager_| to a PasswordFormManager. They use the new
+    // and the old FormData parser, respectively. Log the differences using UKM
+    // to be alerted of regressions early.
+    RecordParsingOnSavingDifference(*provisional_save_manager_,
+                                    *submitted_manager,
+                                    submitted_manager->GetMetricsRecorder());
+  }
+
   if (ShouldPromptUserToSavePassword(*submitted_manager)) {
     bool empty_password =
         submitted_manager->GetPendingCredentials().username_value.empty();

components/password_manager/core/browser/password_manager_unittest.cc

@@ -2782,4 +2782,37 @@ TEST_F(PasswordManagerTest, ManualFallbackForSavingNewParser) {
   manager()->HideManualFallbackForSaving();
 }
 
+// Check that some value for the ParsingOnSavingDifference UKM metric is emitted
+// on a successful login.
+TEST_F(PasswordManagerTest, ParsingOnSavingMetricRecorded) {
+  ukm::TestAutoSetUkmRecorder test_ukm_recorder;
+  base::test::ScopedFeatureList scoped_feature_list;
+  TurnOnNewParsingForSaving(&scoped_feature_list);
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms()));
+
+  PasswordForm form = MakeSimpleForm();
+  std::vector<PasswordForm> observed = {form};
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+
+  // Provisionally save and simulate a successful landing page load to make
+  // manager() believe this password should be saved.
+  manager()->ProvisionallySavePassword(form, nullptr);
+  manager()->OnPasswordFormsRendered(&driver_, {}, true);
+
+  // Destroy |manager_| to send off UKM metrics.
+  manager_.reset();
+
+  std::vector<const ukm::mojom::UkmEntry*> ukm_entries =
+      test_ukm_recorder.GetEntriesByName(
+          ukm::builders::PasswordForm::kEntryName);
+  ASSERT_EQ(1u, ukm_entries.size());
+  test_ukm_recorder.EntryHasMetric(
+      ukm_entries[0],
+      ukm::builders::PasswordForm::kParsingOnSavingDifferenceName);
+}
+
 }  // namespace password_manager

tools/metrics/ukm/ukm.xml

@@ -3127,6 +3127,14 @@ be describing additional metrics about the same event.
       password_manager::PasswordFormMetricsRecorder::ParsingComparisonResult.
     </summary>
   </metric>
+  <metric name="ParsingOnSavingDifference">
+    <summary>
+      Records the comparison of the old and new password form parsing for
+      saving. Multiple values from the
+      password_manager::PasswordFormMetricsRecorder::ParsingOnSavingDifference
+      bitmask can be combined.
+    </summary>
+  </metric>
   <metric name="Saving.Prompt.Interaction">
     <summary>
       Records how the user interacted with a saving prompt. Recorded values