Clean up PasswordForm.FillOnLoad UKM

There was a discussion about whether Chrome should avoid filling on
load if a form has some new-password fields, or if the condition
should be "has no current-password fields".

The PasswordForm.FillOnLoad metric supported a team decision on that.

The decision was to go with the new condition (for the new parser).

Therefore the metric is now marked as obsolete and no longer
collected.

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

components/password_manager/core/browser/DEPS

@@ -36,9 +36,6 @@ specific_include_rules = {
   "password_autofill_manager_unittest\.cc": [
     "+components/ukm",
   ],
-  "password_form_filling_unittest\.cc": [
-    "+components/ukm",
-  ],
   "password_form_manager_unittest\.cc": [
     "+components/ukm",
   ],

components/password_manager/core/browser/password_form_filling.cc

@@ -101,13 +101,6 @@ void ShowInitialPasswordAccountSuggestions(
   driver->ShowInitialPasswordAccountSuggestions(fill_data);
 }
 
-// Returns true if filling the |form| is likely to be useful for the user.
-bool FormGoodForFilling(const PasswordForm& form) {
-  return !form.password_element.empty() ||
-         form.password_element_renderer_id !=
-             autofill::FormFieldData::kNotSetFormControlRendererId;
-}
-
 }  // namespace
 
 void SendFillInformationToRenderer(
@@ -134,53 +127,29 @@ void SendFillInformationToRenderer(
   DCHECK(preferred_match);
 
   // Chrome tries to avoid filling into fields where the user is asked to enter
-  // a fresh password. The old condition for filling on load was: "does the form
-  // lack a new-password field?" There is currently a discussion whether this
-  // should rather be: "does the form have a current-password field?" because
-  // the current-password field is what should be filled. Currently, the old
-  // condition is still used with the old parser, and the new condition with the
-  // new one.
-  // Old condition.
-  const bool did_fill_on_load = !observed_form.IsPossibleChangePasswordForm();
-  // New condition.
-  const bool will_fill_on_load = FormGoodForFilling(observed_form);
+  // a fresh password. The old condition for filling on load was: "does the
+  // form lack a new-password field?" The new one is: "does the form have a
+  // current-password field?" because the current-password field is what should
+  // be filled. The old condition is used with the old parser, and the new
+  // condition with the new one. The new one is not explicitly checked here,
+  // because it is implicit in the way filling is done: if there is no current
+  // password field ID, then PasswordAutofillAgent has no way to fill the
+  // password anywhere.
   const bool form_good_for_filling =
       base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)
-          ? will_fill_on_load
-          : did_fill_on_load;
+          ? true
+          : !observed_form.IsPossibleChangePasswordForm();
 
   // Proceed to autofill.
   // Note that we provide the choices but don't actually prefill a value if:
   // (1) we are in Incognito mode, or
   // (2) if it matched using public suffix domain matching, or
-  // (3) the form is change password form.
+  // (3) it would result in unexpected filling in a form with new password
+  //     fields.
   bool wait_for_username = client.IsIncognito() ||
                            preferred_match->is_public_suffix_match ||
                            !form_good_for_filling;
 
-  // The following metric is only relevant when fill on load is not suppressed
-  // by Incognito or PSL-matched credentials. It is also only relevant for the
-  // new parser, because the tested change will only be launched for that.
-  // TODO(crbug.com/895781): Remove once the password manager team decides
-  // whether to go with the new condition or the old one.
-  if (!client.IsIncognito() && !preferred_match->is_public_suffix_match &&
-      base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)) {
-    PasswordFormMetricsRecorder::FillOnLoad fill_on_load_result =
-        PasswordFormMetricsRecorder::FillOnLoad::kSame;
-    // 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 =
-          PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad;
-    }
-    metrics_recorder->RecordFillOnLoad(fill_on_load_result);
-  }
-
   if (wait_for_username) {
     metrics_recorder->SetManagerAction(
         PasswordFormMetricsRecorder::kManagerActionNone);

components/password_manager/core/browser/password_form_filling_unittest.cc

@@ -18,7 +18,6 @@
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/browser/stub_password_manager_driver.h"
 #include "components/password_manager/core/common/password_manager_features.h"
-#include "components/ukm/test_ukm_recorder.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -149,56 +148,38 @@ TEST_F(PasswordFormFillingTest, Autofill) {
   }
 }
 
-TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
+TEST_F(PasswordFormFillingTest, TestFillOnLoadSuggestion) {
   const struct {
     const char* description;
-    bool new_password_name_empty;
+    bool new_password_present;
     bool current_password_present;
-    PasswordFormMetricsRecorder::FillOnLoad expected_comparison;
   } kTestCases[] = {
       {
-          .description = "Fills on load",
-          .new_password_name_empty = true,
+          .description = "No new, some current",
+          .new_password_present = false,
           .current_password_present = true,
-          .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
       },
       {
-          .description = "Does not fill on load",
-          .new_password_name_empty = false,
+          .description = "No current, some new",
+          .new_password_present = true,
           .current_password_present = false,
-          .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
       },
       {
-          .description = "Did not fill on load, will fill on load",
-          .new_password_name_empty = false,
+          .description = "Both",
+          .new_password_present = true,
           .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);
-  auto task_runner = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
-  metrics_recorder_.reset();  // The recorder will be re-created below.
 
   for (const auto& test_case : kTestCases) {
     SCOPED_TRACE(test_case.description);
-    base::TestMockTimeTaskRunner::ScopedContext scoped_context(
-        task_runner.get());
-    ukm::TestAutoSetUkmRecorder test_ukm_recorder;
-    metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        true, client_.GetUkmSourceId());
     std::map<base::string16, const PasswordForm*> best_matches;
     best_matches[saved_match_.username_value] = &saved_match_;
 
     PasswordForm observed_form = observed_form_;
-    if (!test_case.new_password_name_empty)
+    if (!test_case.new_password_present)
       observed_form.new_password_element = ASCIIToUTF16("New Passwd");
     if (!test_case.current_password_present)
       observed_form.password_element.clear();
@@ -212,17 +193,9 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
                                   best_matches, federated_matches_,
                                   &saved_match_, metrics_recorder_.get());
 
-    EXPECT_EQ(test_case.current_password_present, !fill_data.wait_for_username);
-
-    metrics_recorder_.reset();  // The recorder only reports on destruction.
-    auto entries = test_ukm_recorder.GetEntriesByName(
-        ukm::builders::PasswordForm::kEntryName);
-    ASSERT_EQ(1u, entries.size());
-    const int64_t* reported_value = ukm::TestUkmRecorder::GetEntryMetric(
-        entries[0], ukm::builders::PasswordForm::kFillOnLoadName);
-    ASSERT_TRUE(reported_value);
-    EXPECT_EQ(static_cast<int64_t>(test_case.expected_comparison),
-              *reported_value);
+    // In all cases, fill on load should not be prevented. If there is no
+    // current-password field, the renderer will not fill anyway.
+    EXPECT_FALSE(fill_data.wait_for_username);
   }
 }
 

components/password_manager/core/browser/password_form_metrics_recorder.cc

@@ -308,11 +308,6 @@ void PasswordFormMetricsRecorder::RecordParsingOnSavingDifference(
   ukm_entry_builder_.SetParsingOnSavingDifference(comparison_result);
 }
 
-void PasswordFormMetricsRecorder::RecordFillOnLoad(
-    FillOnLoad comparison_result) {
-  ukm_entry_builder_.SetFillOnLoad(static_cast<int64_t>(comparison_result));
-}
-
 void PasswordFormMetricsRecorder::RecordReadonlyWhenFilling(uint64_t value) {
   ukm_entry_builder_.SetReadonlyWhenFilling(value);
 }

components/password_manager/core/browser/password_form_metrics_recorder.h

@@ -183,25 +183,6 @@ class PasswordFormMetricsRecorder
     kGenerated = 1 << 3,
   };
 
-  // In addition to Incognito mode and PSL credentials, Chrome also does not
-  // fill a form on load when it contains a 'new-password' field. In the
-  // password manager team, we consider replacing this old condition with "no
-  // fill on load if there is no 'current-password' field". This enum is used in
-  // the UKM PasswordForm.FillOnLoad to report which changes in behaviour this
-  // causes.
-  enum class FillOnLoad {
-    // 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,
-    // Obsolete, cannot happen.
-    kObsoleteStopsFillingOnLoad
-  };
-
   // Indicator whether the user has seen a password generation popup and why.
   enum class PasswordGenerationPopupShown {
     kNotShown = 0,
@@ -333,11 +314,6 @@ class PasswordFormMetricsRecorder
   // |comparison_result| is a bitmask of values from ParsingOnSavingDifference.
   void RecordParsingOnSavingDifference(uint64_t comparison_result);
 
-  // Records the comparison of the old and new decision about filling a form on
-  // load. Should only be logged for fillig with non-PSL matched credentials, in
-  // non-Incognito mode.
-  void RecordFillOnLoad(FillOnLoad comparison_result);
-
   // Records the readonly status encoded with parsing success after parsing for
   // filling. The |value| is constructed as follows: The least significant bit
   // says whether parsing succeeded (1) or not (0). The rest, shifted by one

tools/metrics/ukm/ukm.xml

@@ -3445,9 +3445,10 @@ be describing additional metrics about the same event.
   </metric>
   <metric name="FillOnLoad">
     <summary>
-      Records the comparison of the old and new decision about filling a form on
-      load. Should only be logged for fillig with non-PSL matched credentials,
-      in non-Incognito mode. Values are from
+      Deprecated in November 2018, because the decision this metric supported
+      was made. Records the comparison of the old and new decision about filling
+      a form on load. Should only be logged for fillig with non-PSL matched
+      credentials, in non-Incognito mode. Values are from
       password_manager::PasswordFormMetricsRecorder::FillOnLoad.
     </summary>
   </metric>