Skip to content

Commit

Permalink
[UPMLocalPwd] Only compute the account in external methods
Browse files Browse the repository at this point in the history 
This is a pure refactoring.

This CL separates the account computation logic out into the external
(PasswordStore-facing) methods. This makes is easier to apply
account-based changes, since they only need to be done at the external layer.

Bug: 306673712
Change-Id: I2143ae193c7999f5b6425d503519ceec935d5f45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5126851
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1243994}
  • Loading branch information
Ioana Pandele authored and Chromium LUCI CQ committed Jan 8, 2024
1 parent d6a4839 commit cd12618
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 29 deletions.
58 changes: 33 additions & 25 deletions chrome/browser/password_manager/android/password_store_android_backend.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "chrome/browser/password_manager/android/password_manager_lifecycle_helper_impl.h"
#include "chrome/browser/password_manager/android/password_store_android_backend_api_error_codes.h"
#include "chrome/browser/password_manager/android/password_store_android_backend_bridge_helper.h"
#include "chrome/browser/password_manager/android/password_store_android_backend_dispatcher_bridge.h"
#include "chrome/browser/password_manager/android/password_sync_controller_delegate_android.h"
#include "chrome/browser/password_manager/android/password_sync_controller_delegate_bridge_impl.h"
#include "components/autofill/core/common/autofill_regexes.h"
Expand Down Expand Up @@ -635,7 +636,8 @@ void PasswordStoreAndroidBackend::GetAllLoginsWithAffiliationAndBrandingAsync(
void PasswordStoreAndroidBackend::GetAutofillableLoginsAsync(
LoginsOrErrorReply callback) {
GetAutofillableLoginsInternal(
std::move(callback), PasswordStoreOperation::kGetAutofillableLoginsAsync,
GetSyncingAccount(sync_service_), std::move(callback),
PasswordStoreOperation::kGetAutofillableLoginsAsync,
/*delay=*/base::Seconds(0));
}

Expand Down Expand Up @@ -676,7 +678,8 @@ void PasswordStoreAndroidBackend::FillMatchingLoginsAsync(
for (const PasswordFormDigest& form : forms) {
callbacks_chain = base::BindOnce(
&PasswordStoreAndroidBackend::GetLoginsInternal,
weak_ptr_factory_.GetWeakPtr(), std::move(form), include_psl,
weak_ptr_factory_.GetWeakPtr(), GetSyncingAccount(sync_service_),
std::move(form), include_psl,
base::BindOnce(barrier_callback).Then(std::move(callbacks_chain)),
PasswordStoreOperation::kFillMatchingLoginsAsync);
}
Expand Down Expand Up @@ -718,7 +721,8 @@ void PasswordStoreAndroidBackend::UpdateLoginAsync(
PasswordChangesOrErrorReply callback) {
CHECK(!form.blocked_by_user ||
(form.username_value.empty() && form.password_value.empty()));
UpdateLoginInternal(form, std::move(callback));
UpdateLoginInternal(GetSyncingAccount(sync_service_), form,
std::move(callback));
}

void PasswordStoreAndroidBackend::RemoveLoginAsync(
Expand All @@ -740,11 +744,12 @@ void PasswordStoreAndroidBackend::RemoveLoginsByURLAndTimeAsync(
ReportMetricsAndInvokeCallbackForStoreModifications(
MetricInfix("RemoveLoginsByURLAndTimeAsync"), std::move(callback));

std::string account = GetSyncingAccount(sync_service_);
GetAllLoginsForAccountInternal(
GetSyncingAccount(sync_service_),
account,
base::BindOnce(&PasswordStoreAndroidBackend::FilterAndRemoveLogins,
weak_ptr_factory_.GetWeakPtr(), std::move(url_filter),
delete_begin, delete_end,
weak_ptr_factory_.GetWeakPtr(), account,
std::move(url_filter), delete_begin, delete_end,
std::move(record_metrics_and_reply),
PasswordStoreOperation::kRemoveLoginsByURLAndTimeAsync,
/*delay=*/base::Seconds(0)),
Expand All @@ -763,14 +768,14 @@ void PasswordStoreAndroidBackend::RemoveLoginsCreatedBetweenAsync(

GetAllLoginsForAccountInternal(
GetSyncingAccount(sync_service_),
base::BindOnce(&PasswordStoreAndroidBackend::FilterAndRemoveLogins,
weak_ptr_factory_.GetWeakPtr(),
// Include all urls.
base::BindRepeating([](const GURL&) { return true; }),
delete_begin, delete_end,
std::move(record_metrics_and_reply),
PasswordStoreOperation::kRemoveLoginsCreatedBetweenAsync,
/*delay=*/base::Seconds(0)),
base::BindOnce(
&PasswordStoreAndroidBackend::FilterAndRemoveLogins,
weak_ptr_factory_.GetWeakPtr(), GetSyncingAccount(sync_service_),
// Include all urls.
base::BindRepeating([](const GURL&) { return true; }), delete_begin,
delete_end, std::move(record_metrics_and_reply),
PasswordStoreOperation::kRemoveLoginsCreatedBetweenAsync,
/*delay=*/base::Seconds(0)),
PasswordStoreOperation::kRemoveLoginsCreatedBetweenAsync,
/*delay=*/base::Seconds(0));
}
Expand All @@ -796,10 +801,11 @@ void PasswordStoreAndroidBackend::DisableAutoSignInForOriginsAsync(
MetricInfix("DisableAutoSignInForOriginsAsync")),
std::move(completion));

std::string account = GetSyncingAccount(sync_service_);
GetAllLoginsForAccountInternal(
GetSyncingAccount(sync_service_),
account,
base::BindOnce(&PasswordStoreAndroidBackend::FilterAndDisableAutoSignIn,
weak_ptr_factory_.GetWeakPtr(), origin_filter,
weak_ptr_factory_.GetWeakPtr(), account, origin_filter,
std::move(record_metrics_and_run_completion)),
PasswordStoreOperation::kDisableAutoSignInForOriginsAsync,
/*delay=*/base::Seconds(0));
Expand Down Expand Up @@ -841,11 +847,11 @@ base::WeakPtr<PasswordStoreBackend> PasswordStoreAndroidBackend::AsWeakPtr() {
}

void PasswordStoreAndroidBackend::GetAutofillableLoginsInternal(
std::string account,
LoginsOrErrorReply callback,
PasswordStoreOperation operation,
base::TimeDelta delay) {
JobId job_id =
bridge_helper_->GetAutofillableLogins(GetSyncingAccount(sync_service_));
JobId job_id = bridge_helper_->GetAutofillableLogins(std::move(account));
QueueNewJob(job_id, std::move(callback),
MetricInfix("GetAutofillableLoginsAsync"),
PasswordStoreOperation::kGetAutofillableLoginsAsync, delay);
Expand All @@ -862,13 +868,13 @@ void PasswordStoreAndroidBackend::GetAllLoginsForAccountInternal(
}

void PasswordStoreAndroidBackend::GetLoginsInternal(
std::string account,
const PasswordFormDigest& form,
bool include_psl,
LoginsOrErrorReply callback,
PasswordStoreOperation operation) {
JobId job_id = bridge_helper_->GetLoginsForSignonRealm(
FormToSignonRealmQuery(form, include_psl),
GetSyncingAccount(sync_service_));
FormToSignonRealmQuery(form, include_psl), std::move(account));
// TODO(crbug.com/1491084): Re-design metrics to be less reliant on exact
// method name and separate external methods from internal ones.
QueueNewJob(job_id,
Expand All @@ -879,10 +885,10 @@ void PasswordStoreAndroidBackend::GetLoginsInternal(
}

void PasswordStoreAndroidBackend::UpdateLoginInternal(
std::string account,
const PasswordForm& form,
PasswordChangesOrErrorReply callback) {
JobId job_id =
bridge_helper_->UpdateLogin(form, GetSyncingAccount(sync_service_));
JobId job_id = bridge_helper_->UpdateLogin(form, std::move(account));
QueueNewJob(job_id, std::move(callback), MetricInfix("UpdateLoginAsync"),
PasswordStoreOperation::kUpdateLoginAsync,
/*delay=*/base::Seconds(0));
Expand Down Expand Up @@ -989,6 +995,7 @@ void PasswordStoreAndroidBackend::OnError(JobId job_id,
base::BindOnce(
&PasswordStoreAndroidBackend::GetAutofillableLoginsInternal,
weak_ptr_factory_.GetWeakPtr(),
GetSyncingAccount(sync_service_),
std::move(*reply).Get<LoginsOrErrorReply>(), operation),
delay);
return;
Expand Down Expand Up @@ -1070,6 +1077,7 @@ PasswordStoreAndroidBackend::GetAndEraseJob(JobId job_id) {
}

void PasswordStoreAndroidBackend::FilterAndRemoveLogins(
std::string account,
const base::RepeatingCallback<bool(const GURL&)>& url_filter,
base::Time delete_begin,
base::Time delete_end,
Expand Down Expand Up @@ -1103,15 +1111,15 @@ void PasswordStoreAndroidBackend::FilterAndRemoveLogins(
for (const auto& login : logins_to_remove) {
callbacks_chain = base::BindOnce(
&PasswordStoreAndroidBackend::RemoveLoginInternal,
weak_ptr_factory_.GetWeakPtr(), GetSyncingAccount(sync_service_),
std::move(login),
weak_ptr_factory_.GetWeakPtr(), account, std::move(login),
base::BindOnce(barrier_callback).Then(std::move(callbacks_chain)),
operation, delay);
}
std::move(callbacks_chain).Run();
}

void PasswordStoreAndroidBackend::FilterAndDisableAutoSignIn(
std::string account,
const base::RepeatingCallback<bool(const GURL&)>& origin_filter,
PasswordChangesOrErrorReply completion,
LoginsResultOrError result) {
Expand Down Expand Up @@ -1142,7 +1150,7 @@ void PasswordStoreAndroidBackend::FilterAndDisableAutoSignIn(
(login.username_value.empty() && login.password_value.empty()));
callbacks_chain = base::BindOnce(
&PasswordStoreAndroidBackend::UpdateLoginInternal,
weak_ptr_factory_.GetWeakPtr(), std::move(login),
weak_ptr_factory_.GetWeakPtr(), account, std::move(login),
base::BindOnce(barrier_callback).Then(std::move(callbacks_chain)));
}
std::move(callbacks_chain).Run();
Expand Down
12 changes: 8 additions & 4 deletions chrome/browser/password_manager/android/password_store_android_backend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ class PasswordStoreAndroidBackend
// PasswordStoreOperation that invoked this method and |delay| is the amount
// of time by which the call to this method was delayed. Calls
// GetAutofillableLogins from the PasswordStoreAndroidBackendDispatcherBridge.
void GetAutofillableLoginsInternal(LoginsOrErrorReply callback,
void GetAutofillableLoginsInternal(std::string account,
LoginsOrErrorReply callback,
PasswordStoreOperation operation,
base::TimeDelta delay);

Expand All @@ -232,21 +233,22 @@ class PasswordStoreAndroidBackend
base::TimeDelta delay);

// Gets logins matching |form|.
void GetLoginsInternal(const PasswordFormDigest& form,
void GetLoginsInternal(std::string account,
const PasswordFormDigest& form,
bool include_psl,
LoginsOrErrorReply callback,
PasswordStoreOperation operation);

// Updates the form in storage with |form|.
void UpdateLoginInternal(const PasswordForm& form,
void UpdateLoginInternal(std::string account,
const PasswordForm& form,
PasswordChangesOrErrorReply callback);

// Removes |form|.
// |operation| is the PasswordStoreOperation that invoked this method and
// |delay| is the amount of time by which the call to this method was delayed.
void RemoveLoginInternal(std::string account,
const PasswordForm& form,

PasswordChangesOrErrorReply callback,
PasswordStoreOperation operation,
base::TimeDelta delay);
Expand Down Expand Up @@ -286,6 +288,7 @@ class PasswordStoreAndroidBackend
// |operation| is the PasswordStoreOperation that invoked this method and
// |delay| is the amount of time by which the call to this method was delayed.
void FilterAndRemoveLogins(
std::string account,
const base::RepeatingCallback<bool(const GURL&)>& url_filter,
base::Time delete_begin,
base::Time delete_end,
Expand All @@ -297,6 +300,7 @@ class PasswordStoreAndroidBackend
// Filters logins that match |origin_filer| and asynchronously disables
// autosignin by updating stored logins.
void FilterAndDisableAutoSignIn(
std::string account,
const base::RepeatingCallback<bool(const GURL&)>& origin_filter,
PasswordChangesOrErrorReply completion,
LoginsResultOrError result);
Expand Down

0 comments on commit cd12618

Please sign in to comment.