Skip to content

GitLab

T
tangled
Commit c68933a3 authored by Vadym Doroshenko's avatar Vadym Doroshenko Committed by Commit Bot
Browse files
Options

Do not show saving manual fallback when fetching is not finished. 

This CL reproduces the same behaviour with NewPasswordFormManager as for
PasswordFormManager.

Bug: 831123
Change-Id: I7ade6f857adfb1d4ec09a8a4333f4e90e9e485b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1619688Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661342}
parent c91ce620
Hide whitespace changes
Inline Side-by-side
Showing with 52 additions and 36 deletions
components/password_manager/core/browser/password_manager.cc
View file @ c68933a3
......@@ -647,7 +647,7 @@ void PasswordManager::OnPasswordFormSubmitted(
password_manager::PasswordManagerDriver* driver,
const PasswordForm& password_form) {
if (IsNewFormParsingForSavingEnabled())
ProvisionallySaveForm(password_form.form_data, driver);
ProvisionallySaveForm(password_form.form_data, driver, false);
ProvisionallySavePassword(password_form, driver);
}
......@@ -683,7 +683,7 @@ void PasswordManager::OnPasswordFormSubmittedNoChecks(
}
if (IsNewFormParsingForSavingEnabled())
ProvisionallySaveForm(password_form.form_data, driver);
ProvisionallySaveForm(password_form.form_data, driver, false);
ProvisionallySavePassword(password_form, driver);
......@@ -701,10 +701,10 @@ void PasswordManager::ShowManualFallbackForSaving(
!client_->GetStoreResultFilter()->ShouldSave(password_form))
return;
std::unique_ptr<PasswordFormManagerInterface> manager = nullptr;
std::unique_ptr<PasswordFormManagerInterface> manager;
if (IsNewFormParsingForSavingEnabled()) {
NewPasswordFormManager* matched_manager =
ProvisionallySaveForm(password_form.form_data, driver);
ProvisionallySaveForm(password_form.form_data, driver, true);
manager = matched_manager ? matched_manager->Clone() : nullptr;
} else {
manager = FindAndCloneMatchedPasswordFormManager(
......@@ -915,7 +915,8 @@ NewPasswordFormManager* PasswordManager::CreateFormManager(
NewPasswordFormManager* PasswordManager::ProvisionallySaveForm(
const FormData& submitted_form,
PasswordManagerDriver* driver) {
PasswordManagerDriver* driver,
bool is_manual_fallback) {
std::unique_ptr<BrowserSavePasswordProgressLogger> logger;
if (password_manager_util::IsLoggingActive(client_)) {
logger.reset(
......@@ -965,6 +966,12 @@ NewPasswordFormManager* PasswordManager::ProvisionallySaveForm(
matched_manager = CreateFormManager(driver, submitted_form);
}
if (is_manual_fallback && matched_manager->GetFormFetcher()->GetState() ==
FormFetcher::State::WAITING) {
// In case of manual fallback, the form manager has to be ready for saving.
return nullptr;
}
if (!matched_manager->ProvisionallySave(submitted_form, driver))
return nullptr;
......
components/password_manager/core/browser/password_manager.h
View file @ c68933a3
......@@ -278,10 +278,13 @@ class PasswordManager : public LoginModel, public FormSubmissionObserver {
// the match. If the function is called multiple times, only the form from the
// last call is provisionally saved. Multiple calls is possible because it is
// called on any user keystroke. If there is no NewPasswordFormManager that
// manages |form|, the new one is created.
// Returns manager which manages |form|.
// manages |form|, the new one is created. If |is_manual_fallback| is true
// and the matched form manager has not recieved yet response from the
// password store, then nullptr is returned. Returns manager which manages
// |form|.
NewPasswordFormManager* ProvisionallySaveForm(const autofill::FormData& form,
PasswordManagerDriver* driver);
PasswordManagerDriver* driver,
bool is_manual_fallback);
// Passes |form| to NewPasswordFormManager that manages it for using it after
// detecting submission success for saving.
......
components/password_manager/core/browser/password_manager_unittest.cc
View file @ c68933a3
......@@ -463,8 +463,7 @@ class PasswordManagerTest : public testing::Test {
manager_.reset(new PasswordManager(&client_));
}
void TurnOnOnlyNewPassword(
base::test::ScopedFeatureList* scoped_feature_list) {
void TurnOnOnlyNewParser(base::test::ScopedFeatureList* scoped_feature_list) {
scoped_feature_list->InitWithFeatures(
{features::kNewPasswordFormParsing,
features::kNewPasswordFormParsingForSaving, features::kOnlyNewParser},
......@@ -1007,7 +1006,7 @@ TEST_F(PasswordManagerTest, FormSubmit) {
TEST_F(PasswordManagerTest, IsPasswordFieldDetectedOnPage) {
base::test::ScopedFeatureList scoped_feature_list;
TurnOnOnlyNewPassword(&scoped_feature_list);
TurnOnOnlyNewParser(&scoped_feature_list);
PasswordForm form(MakeSimpleForm());
EXPECT_CALL(*store_, GetLogins(_, _))
.WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms()));
......@@ -2656,31 +2655,38 @@ TEST_F(PasswordManagerTest, ManualFallbackForSaving) {
// Tests that the manual fallback for saving isn't shown if there is no response
// from the password storage. When crbug.com/741537 is fixed, change this test.
TEST_F(PasswordManagerTest, ManualFallbackForSaving_SlowBackend) {
std::vector<PasswordForm> observed;
PasswordForm form(MakeSimpleForm());
observed.push_back(form);
PasswordStoreConsumer* store_consumer = nullptr;
EXPECT_CALL(client_, IsSavingAndFillingEnabled(form.origin))
.WillRepeatedly(Return(true));
// TODO(https://crbug.com/949519): replace WillRepeatedly with WillOnce when
// the old parser is gone.
EXPECT_CALL(*store_, GetLogins(_, _))
.WillRepeatedly(SaveArg<1>(&store_consumer));
manager()->OnPasswordFormsParsed(&driver_, observed);
manager()->OnPasswordFormsRendered(&driver_, observed, true);
for (bool only_new_parser_enabled : {false, true}) {
base::test::ScopedFeatureList scoped_feature_list;
if (only_new_parser_enabled)
TurnOnOnlyNewParser(&scoped_feature_list);
std::vector<PasswordForm> observed;
PasswordForm form(MakeSimpleForm());
observed.push_back(form);
PasswordStoreConsumer* store_consumer = nullptr;
EXPECT_CALL(client_, IsSavingAndFillingEnabled(form.origin))
.WillRepeatedly(Return(true));
// TODO(https://crbug.com/949519): replace WillRepeatedly with WillOnce when
// the old parser is gone.
EXPECT_CALL(*store_, GetLogins(_, _))
.WillRepeatedly(SaveArg<1>(&store_consumer));
manager()->OnPasswordFormsParsed(&driver_, observed);
manager()->OnPasswordFormsRendered(&driver_, observed, true);
// There is no response from the store. Don't show the fallback.
EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, _, _)).Times(0);
manager()->ShowManualFallbackForSaving(&driver_, form);
// There is no response from the store. Don't show the fallback.
EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, _, _)).Times(0);
manager()->ShowManualFallbackForSaving(&driver_, form);
// The storage responded. The fallback can be shown.
ASSERT_TRUE(store_consumer);
store_consumer->OnGetPasswordStoreResults(
std::vector<std::unique_ptr<PasswordForm>>());
std::unique_ptr<PasswordFormManagerForUI> form_manager_to_save;
EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, false, false))
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save)));
manager()->ShowManualFallbackForSaving(&driver_, form);
// The storage responded. The fallback can be shown.
ASSERT_TRUE(store_consumer);
store_consumer->OnGetPasswordStoreResults(
std::vector<std::unique_ptr<PasswordForm>>());
std::unique_ptr<PasswordFormManagerForUI> form_manager_to_save;
EXPECT_CALL(client_, ShowManualFallbackForSavingPtr(_, false, false))
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save)));
manager()->ShowManualFallbackForSaving(&driver_, form);
Mock::VerifyAndClearExpectations(&client_);
Mock::VerifyAndClearExpectations(&store_);
}
}
TEST_F(PasswordManagerTest, ManualFallbackForSaving_GeneratedPassword) {
......@@ -3006,7 +3012,7 @@ TEST_F(PasswordManagerTest, ProcessingNormalFormSubmission) {
<< " successful_submission = " << successful_submission);
base::test::ScopedFeatureList scoped_feature_list;
if (only_new_parser)
TurnOnOnlyNewPassword(&scoped_feature_list);
TurnOnOnlyNewParser(&scoped_feature_list);
else
TurnOnNewParsingForSaving(&scoped_feature_list, true);
......@@ -3060,7 +3066,7 @@ TEST_F(PasswordManagerTest, ProcessingOtherSubmissionTypes) {
SCOPED_TRACE(testing::Message("only_new_parser = ") << only_new_parser);
base::test::ScopedFeatureList scoped_feature_list;
if (only_new_parser)
TurnOnOnlyNewPassword(&scoped_feature_list);
TurnOnOnlyNewParser(&scoped_feature_list);
else
TurnOnNewParsingForSaving(&scoped_feature_list, true);
......
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