Commit 722eff78 authored by Vaclav Brozek's avatar Vaclav Brozek Committed by Commit Bot

Start filling change password forms

Chrome currently won't autofill credentials into forms which have a
"new-password" field.

This includes change-password forms, which also have the
"current-password" field. Filling that is actually useful for users.
Therefore this CL changes the condition aginst autofilling to only
pick forms which do not have the "current-password" field.

This is limited to the kNewPasswordFormParsing feature, to make
it easier to understand the impact of that change to the overall
difference between the old parser and the new parser.

Bug: 895781
Change-Id: I39cee16c725d108213e045f0cef94f52a6e80837
Reviewed-on: https://chromium-review.googlesource.com/c/1309742
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606964}
parent 57de6d5b
......@@ -133,6 +133,22 @@ 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);
const bool form_good_for_filling =
base::FeatureList::IsEnabled(features::kNewPasswordFormParsing)
? will_fill_on_load
: did_fill_on_load;
// Proceed to autofill.
// Note that we provide the choices but don't actually prefill a value if:
// (1) we are in Incognito mode, or
......@@ -140,7 +156,7 @@ void SendFillInformationToRenderer(
// (3) the form is change password form.
bool wait_for_username = client.IsIncognito() ||
preferred_match->is_public_suffix_match ||
observed_form.IsPossibleChangePasswordForm();
!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
......@@ -149,10 +165,6 @@ void SendFillInformationToRenderer(
// 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) {
......
......@@ -198,13 +198,16 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadReported) {
observed_form.password_element.clear();
EXPECT_CALL(driver_, AllowPasswordGenerationForForm(observed_form));
EXPECT_CALL(driver_, FillPasswordForm(_));
PasswordFormFillData fill_data;
EXPECT_CALL(driver_, FillPasswordForm(_)).WillOnce(SaveArg<0>(&fill_data));
EXPECT_CALL(client_, PasswordWasAutofilled(_, _, _));
SendFillInformationToRenderer(client_, &driver_, false, observed_form,
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);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment