Add UKM: PasswordForm.FillOnLoad

Chrome currently won't autofill credentials into forms which have a
"new-password" field. A better criterion seems to be: only fill into
forms with a "current-password" field.

This CL adds a UKM metric to understand the impact of such change.

Privacy review of this metric:
The whole UKM event "PasswordForm" has been reviewed in
crbug.com/728707 and go/gkmnc, where the privacy TL agreed that adding
new metrics for this event is OK under certain conditions (see the
linked doc). The new metric, which measures changes to filling on load
of forms for which the user previously stored some credentials,
satisfies such conditions, in the opinion of the CL author, and hence
is covered by that review. The new metric has been added to the
requested spreadsheet listing all passwords-related metrics.

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

components/password_manager/core/browser/DEPS

@@ -36,6 +36,9 @@ 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,6 +101,13 @@ 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,6 +141,29 @@ void SendFillInformationToRenderer(
   bool wait_for_username = client.IsIncognito() ||
                            preferred_match->is_public_suffix_match ||
                            observed_form.IsPossibleChangePasswordForm();
+
+  // 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)) {
+    // Old condition.
+    const bool did_fill_on_load = !observed_form.IsPossibleChangePasswordForm();
+    // New condition.
+    const bool will_fill_on_load = FormGoodForFilling(observed_form);
+    PasswordFormMetricsRecorder::FillOnLoad fill_on_load_result =
+        PasswordFormMetricsRecorder::FillOnLoad::kSame;
+    if (did_fill_on_load != will_fill_on_load) {
+      fill_on_load_result =
+          will_fill_on_load
+              ? PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad
+              : PasswordFormMetricsRecorder::FillOnLoad::kStopsFillingOnLoad;
+    }
+    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

@@ -10,11 +10,15 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_form_fill_data.h"
 #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
 #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"
@@ -145,6 +149,74 @@ TEST_F(PasswordFormFillingTest, Autofill) {
   }
 }
 
+TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
+  const struct {
+    const char* description;
+    bool new_password_present;
+    bool current_password_present;
+    PasswordFormMetricsRecorder::FillOnLoad expected_comparison;
+  } kTestCases[] = {
+      {
+          .description = "Fills on load",
+          .new_password_present = false,
+          .current_password_present = true,
+          .expected_comparison = PasswordFormMetricsRecorder::FillOnLoad::kSame,
+      },
+      {
+          .description = "Does not fill on load",
+          .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_present = true,
+          .current_password_present = true,
+          .expected_comparison =
+              PasswordFormMetricsRecorder::FillOnLoad::kStartsFillingOnLoad,
+      },
+  };
+  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_present)
+      observed_form.new_password_element = ASCIIToUTF16("New Passwd");
+    if (!test_case.current_password_present)
+      observed_form.password_element.clear();
+
+    EXPECT_CALL(driver_, AllowPasswordGenerationForForm(observed_form));
+    EXPECT_CALL(driver_, FillPasswordForm(_));
+    EXPECT_CALL(client_, PasswordWasAutofilled(_, _, _));
+
+    SendFillInformationToRenderer(client_, &driver_, false, observed_form,
+                                  best_matches, federated_matches_,
+                                  &saved_match_, metrics_recorder_.get());
+
+    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);
+  }
+}
+
 TEST_F(PasswordFormFillingTest, AutofillPSLMatch) {
   std::map<base::string16, const autofill::PasswordForm*> best_matches;
   best_matches[saved_match_.username_value] = &psl_saved_match_;

components/password_manager/core/browser/password_form_metrics_recorder.cc

@@ -308,6 +308,11 @@ 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,6 +183,24 @@ 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 {
+    // The old and the new condition evaluate to the same result.
+    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
+  };
+
   // Indicator whether the user has seen a password generation popup and why.
   enum class PasswordGenerationPopupShown {
     kNotShown = 0,
@@ -314,6 +332,11 @@ 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

@@ -3442,6 +3442,14 @@ be describing additional metrics about the same event.
       distinguish two forms on the same site.
     </summary>
   </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
+      password_manager::PasswordFormMetricsRecorder::FillOnLoad.
+    </summary>
+  </metric>
   <metric name="Generation.GeneratedPassword">
     <summary>
       Records '1' if the user has generated a password on this form.