Fix PasswordForm.FillOnLoad UKM

The PasswordForm.FillOnLoad UKM aims at measuring the change in
behaviour of Chrome with the new password form parser, if an explicit
condition for filling on load is changed from A to B.

It turned out that even if A is the explicit condition, B still can
break fill on load. So the change is not so much A -> B as A && B ->
B. This means that the metric's bucket recording the state when A is
true and B false needs to be merged into the bucket when A == B,
because in both cases the behaviour does not change.

Bug: 895781
Change-Id: Ic3ab14d9a1e43c0312580d2dfe5bff34f2cdd20a
Reviewed-on: https://chromium-review.googlesource.com/c/1340274Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609221}

components/password_manager/core/browser/password_form_filling.cc

@@ -167,11 +167,16 @@ void SendFillInformationToRenderer(
       base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)) {
     PasswordFormMetricsRecorder::FillOnLoad fill_on_load_result =
         PasswordFormMetricsRecorder::FillOnLoad::kSame;
-    if (did_fill_on_load != will_fill_on_load) {
+    // Note: The fill on load never happens if |will_fill_on_load| is false,
+    // because PasswordAutofillAgent won't be able to locate the "current
+    // password" field to fill. So even if |did_fill_on_load| is true and
+    // |will_fill_on_load| is false, the behaviour of Chrome does not change if
+    // either of them is picked for |form_good_for_filling|. So the only
+    // interesting case to report is when |did...| is false and |will...| is
+    // true.
+    if (!did_fill_on_load && will_fill_on_load) {
       fill_on_load_result =
-          will_fill_on_load
-              ? PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad
-              : PasswordFormMetricsRecorder::FillOnLoad::kStopsFillingOnLoad;
+          PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad;
     }
     metrics_recorder->RecordFillOnLoad(fill_on_load_result);
   }

components/password_manager/core/browser/password_form_filling_unittest.cc

@@ -152,29 +152,35 @@ TEST_F(PasswordFormFillingTest, Autofill) {
 TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
   const struct {
     const char* description;
-    bool new_password_present;
+    bool new_password_name_empty;
     bool current_password_present;
     PasswordFormMetricsRecorder::FillOnLoad expected_comparison;
   } kTestCases[] = {
       {
           .description = "Fills on load",
-          .new_password_present = false,
+          .new_password_name_empty = true,
           .current_password_present = true,
           .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
       },
       {
           .description = "Does not fill on load",
-          .new_password_present = true,
+          .new_password_name_empty = false,
           .current_password_present = false,
           .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
       },
       {
           .description = "Did not fill on load, will fill on load",
-          .new_password_present = true,
+          .new_password_name_empty = false,
           .current_password_present = true,
           .expected_comparison =
               PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad,
       },
+      {
+          .description = "New password field present but its name is empty",
+          .new_password_name_empty = true,
+          .current_password_present = false,
+          .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
+      },
   };
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(features::kNewPasswordFormParsing);
@@ -192,7 +198,7 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
     best_matches[saved_match_.username_value] = &saved_match_;
 
     PasswordForm observed_form = observed_form_;
-    if (test_case.new_password_present)
+    if (!test_case.new_password_name_empty)
       observed_form.new_password_element = ASCIIToUTF16("New Passwd");
     if (!test_case.current_password_present)
       observed_form.password_element.clear();

components/password_manager/core/browser/password_form_metrics_recorder.h

@@ -190,15 +190,16 @@ class PasswordFormMetricsRecorder
   // the UKM PasswordForm.FillOnLoad to report which changes in behaviour this
   // causes.
   enum class FillOnLoad {
-    // The old and the new condition evaluate to the same result.
+    // No change in fill-on-load behaviour, either because the old and the new
+    // condition evaluate to the same result, or because the new condition being
+    // false breaks fill-on-load even in the case when only the old condition is
+    // checked.
     kSame,
     // The old condition prevented fill on load, the new allows it. This happens
     // for forms with both 'current-password' and 'new-password' fields.
     kStartsFillingOnLoad,
-    // The old condition allowed filling on load, the new prevents it. This
-    // corresponds to forms with neither 'current-password' nor 'new-password'
-    // fields, so it should never happen. It is included for completeness.
-    kStopsFillingOnLoad
+    // Obsolete, cannot happen.
+    kObsoleteStopsFillingOnLoad
   };
 
   // Indicator whether the user has seen a password generation popup and why.