Skip to content

Commit

Permalink
Remove wallet address conversion logic
Browse files Browse the repository at this point in the history 
The wallet address conversion logic got deprecated with the launch of
Buttered Leipzig. See crrev.com/c/4742310. This CL cleans up all the
related code.

OBSOLETE_HISTOGRAM[Autofill.WalletAddressConversionType]=Got deprecated with the launch of Buttered Leipzig.

Bug: 1348294
Change-Id: Ida5b543af1ac9df376632104489d72f97c402f4e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4922357
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Commit-Queue: Vidhan Jain <vidhanj@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1210219}
  • Loading branch information
Vidhan authored and Chromium LUCI CQ committed Oct 16, 2023
1 parent 929b789 commit 712c2a3
Show file tree
Hide file tree
Showing 17 changed files with 5 additions and 528 deletions.
2 changes: 0 additions & 2 deletions components/autofill/core/browser/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,8 +526,6 @@ static_library("browser") {
"webdata/autofill_webdata_backend.h",
"webdata/autofill_webdata_backend_impl.cc",
"webdata/autofill_webdata_backend_impl.h",
"webdata/autofill_webdata_backend_util.cc",
"webdata/autofill_webdata_backend_util.h",
"webdata/autofill_webdata_service.cc",
"webdata/autofill_webdata_service.h",
"webdata/autofill_webdata_service_observer.h",
Expand Down
8 changes: 0 additions & 8 deletions components/autofill/core/browser/metrics/autofill_metrics.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2051,14 +2051,6 @@ void AutofillMetrics::LogIsQueriedCreditCardFormSecure(bool is_secure) {
UMA_HISTOGRAM_BOOLEAN("Autofill.QueriedCreditCardFormIsSecure", is_secure);
}

// static
void AutofillMetrics::LogWalletAddressConversionType(
WalletAddressConversionType type) {
DCHECK_LT(type, NUM_CONVERTED_ADDRESS_CONVERSION_TYPES);
UMA_HISTOGRAM_ENUMERATION("Autofill.WalletAddressConversionType", type,
NUM_CONVERTED_ADDRESS_CONVERSION_TYPES);
}

// static
void AutofillMetrics::LogShowedHttpNotSecureExplanation() {
base::RecordAction(
Expand Down
13 changes: 0 additions & 13 deletions components/autofill/core/browser/metrics/autofill_metrics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,15 +577,6 @@ class AutofillMetrics {
NUM_WALLET_REQUIRED_ACTIONS
};

// For measuring how wallet addresses are converted to local profiles.
enum WalletAddressConversionType : int {
// The converted wallet address was merged into an existing local profile.
CONVERTED_ADDRESS_MERGED,
// The converted wallet address was added as a new local profile.
CONVERTED_ADDRESS_ADDED,
NUM_CONVERTED_ADDRESS_CONVERSION_TYPES
};

// To record whether the upload event was sent.
enum class UploadEventStatus { kNotSent, kSent, kMaxValue = kSent };

Expand Down Expand Up @@ -1208,10 +1199,6 @@ class AutofillMetrics {
// context.
static void LogIsQueriedCreditCardFormSecure(bool is_secure);

// Log how the converted wallet address was added to the local autofill
// profiles.
static void LogWalletAddressConversionType(WalletAddressConversionType type);

// This should be called when the user selects the Form-Not-Secure warning
// suggestion to show an explanation of the warning.
static void LogShowedHttpNotSecureExplanation();
Expand Down
21 changes: 1 addition & 20 deletions components/autofill/core/browser/personal_data_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,13 +644,7 @@ void PersonalDataManager::OnWebDataServiceRequestDone(

void PersonalDataManager::OnAutofillChangedBySync(
syncer::ModelType model_type) {
// After each change coming from sync we go through a two-step process:
// - First, we post a task on the DB sequence to (potentially) convert server
// addresses to local addresses and update cards accordingly.
// - This conversion task is concluded by a
// AutofillAddressConversionCompleted() notification. As a second step, we
// need to refresh the PDM's view of the data.
ConvertWalletAddressesAndUpdateWalletCards();
Refresh();

// Note, it's possible that the cleanups are run on the stale data since
// `Refresh` is an async operation. But, since the cleanups happen over the
Expand All @@ -659,10 +653,6 @@ void PersonalDataManager::OnAutofillChangedBySync(
model_type);
}

void PersonalDataManager::AutofillAddressConversionCompleted() {
Refresh();
}

void PersonalDataManager::OnStateChanged(syncer::SyncService* sync_service) {
DCHECK_EQ(sync_service_, sync_service);

Expand Down Expand Up @@ -2510,15 +2500,6 @@ void PersonalDataManager::NotifyPersonalDataObserver() {

void PersonalDataManager::OnCreditCardSaved(bool is_local_card) {}

void PersonalDataManager::ConvertWalletAddressesAndUpdateWalletCards() {
// PDM expects that each call to
// ConvertWalletAddressesAndUpdateWalletCards() is followed by a
// AutofillAddressConversionCompleted() notification, simulate the
// notification here.
// TODO(crbug.com/1348294): Simplify this code.
AutofillAddressConversionCompleted();
}

void PersonalDataManager::UpdateProfileInDB(const AutofillProfile& profile) {
if (!ProfileChangesAreOngoing(profile.guid())) {
const auto* existing_profile = GetProfileByGUID(profile.guid());
Expand Down
28 changes: 0 additions & 28 deletions components/autofill/core/browser/personal_data_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ class PersonalDataManager : public KeyedService,

// AutofillWebDataServiceObserverOnUISequence:
void OnAutofillChangedBySync(syncer::ModelType model_type) override;
void AutofillAddressConversionCompleted() override;

// SyncServiceObserver:
void OnStateChanged(syncer::SyncService* sync) override;
Expand Down Expand Up @@ -842,33 +841,6 @@ class PersonalDataManager : public KeyedService,
// prefs::kAutofillProfileEnabled changes.
void EnableAutofillPrefChanged();

// Converts the Wallet addresses to local autofill profiles. This should be
// called after all the syncable data has been processed (local cards and
// profiles, Wallet data and metadata). Also updates Wallet cards' billing
// address id to point to the local profiles.
void ConvertWalletAddressesAndUpdateWalletCards();

// Converts the Wallet addresses into local profiles either by merging with an
// existing |local_profiles| of by adding a new one. Populates the
// |server_id_profiles_map| to be used when updating cards where the address
// was already converted. Also populates the |guids_merge_map| to keep the
// link between the Wallet address and the equivalent local profile (from
// merge or creation).
bool ConvertWalletAddressesToLocalProfiles(
std::vector<AutofillProfile>* local_profiles,
std::unordered_map<std::string, AutofillProfile*>* server_id_profiles_map,
std::unordered_map<std::string, std::string>* guids_merge_map);

// Goes through the Wallet cards to find cards where the billing address is a
// Wallet address which was already converted in a previous pass. Looks for a
// matching local profile and updates the |guids_merge_map| to make the card
// refer to it.
bool UpdateWalletCardsAlreadyConvertedBillingAddresses(
const std::vector<AutofillProfile>& local_profiles,
const std::unordered_map<std::string, AutofillProfile*>&
server_id_profiles_map,
std::unordered_map<std::string, std::string>* guids_merge_map) const;

// Removes profile from web database according to |guid| and resets credit
// card's billing address if that address is used by any credit cards.
// The method does not refresh, this allows multiple removal with one
Expand Down
71 changes: 0 additions & 71 deletions components/autofill/core/browser/personal_data_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,17 +309,6 @@ class PersonalDataManagerHelper : public PersonalDataManagerTestBase {
test::SetServerCreditCards(GetServerDataTable(), server_cards);
}

void SetServerProfiles(const std::vector<AutofillProfile>& server_profiles) {
GetServerDataTable()->SetServerProfiles(server_profiles);
}

void ConvertWalletAddressesAndUpdateWalletCards() {
// Simulate new data is coming from sync which triggers a conversion of
// wallet addresses which in turn triggers a refresh.
personal_data_->OnAutofillChangedBySync(syncer::AUTOFILL_WALLET_DATA);
PersonalDataProfileTaskWaiter(*personal_data_).Wait();
}

void AddOfferDataForTest(AutofillOfferData offer_data) {
personal_data_->AddOfferDataForTest(
std::make_unique<AutofillOfferData>(offer_data));
Expand Down Expand Up @@ -2139,7 +2128,6 @@ TEST_F(PersonalDataManagerTest, GetProfilesToSuggest_ProfileAutofillDisabled) {
// Disable Profile autofill.
prefs::SetAutofillProfileEnabled(personal_data_->pref_service_, false);
PersonalDataProfileTaskWaiter(*personal_data_).Wait();
ConvertWalletAddressesAndUpdateWalletCards();

// Check that profiles were saved.
const size_t expected_profiles = 1;
Expand All @@ -2164,7 +2152,6 @@ TEST_F(PersonalDataManagerTest,

personal_data_->Refresh();
PersonalDataProfileTaskWaiter(*personal_data_).Wait();
ConvertWalletAddressesAndUpdateWalletCards();

// Expect that all profiles are suggested.
const size_t expected_profiles = 1;
Expand Down Expand Up @@ -2916,64 +2903,6 @@ TEST_F(PersonalDataManagerTest, DeleteAllLocalCreditCards) {
EXPECT_EQ(0U, personal_data_->GetLocalCreditCards().size());
}

// Tests that Wallet addresses do NOT get converted if they're stored in
// ephemeral storage.
TEST_F(PersonalDataManagerSyncTransportModeTest,
DoNotConvertWalletAddressesInEphemeralStorage) {
///////////////////////////////////////////////////////////////////////
// Setup.
///////////////////////////////////////////////////////////////////////
ASSERT_FALSE(personal_data_->IsSyncFeatureEnabledForPaymentsServerMetrics());

// Add a local profile.
AutofillProfile local_profile;
test::SetProfileInfo(&local_profile, "Josephine", "Alicia", "Saenz", "",
"Fox", "1212 Center.", "Bld. 5", "", "", "", "", "");
AddProfileToPersonalDataManager(local_profile);

// Add two server profiles: The first is unique, the second is similar to the
// local one but has some additional info.
std::vector<AutofillProfile> server_profiles;
server_profiles.emplace_back(AutofillProfile::SERVER_PROFILE,
"server_address1");
test::SetProfileInfo(&server_profiles.back(), "John", "", "Doe", "", "",
"1212 Center", "Bld. 5", "Orlando", "FL", "32801", "US",
"");
server_profiles.back().SetRawInfo(NAME_FULL, u"John Doe");

server_profiles.emplace_back(AutofillProfile::SERVER_PROFILE,
"server_address2");
test::SetProfileInfo(&server_profiles.back(), "Josephine", "Alicia", "Saenz",
"joewayne@me.xyz", "Fox", "1212 Center.", "Bld. 5",
"Orlando", "FL", "32801", "US", "19482937549");
server_profiles.back().SetRawInfo(NAME_FULL, u"Josephine Alicia Saenz");
SetServerProfiles(server_profiles);

ASSERT_TRUE(AutofillProfileComparator(personal_data_->app_locale())
.AreMergeable(local_profile, server_profiles.back()));

// Make sure everything is set up correctly.
personal_data_->Refresh();
PersonalDataProfileTaskWaiter(*personal_data_).Wait();
ASSERT_EQ(1U, personal_data_->GetProfiles().size());
ASSERT_EQ(2U, personal_data_->GetServerProfiles().size());

///////////////////////////////////////////////////////////////////////
// Tested method.
///////////////////////////////////////////////////////////////////////
// Since the wallet addresses are in ephemeral storage, they should *not* get
// converted to local addresses.
ConvertWalletAddressesAndUpdateWalletCards();

///////////////////////////////////////////////////////////////////////
// Validation.
///////////////////////////////////////////////////////////////////////
// There should be no changes to the local profiles: No new one added, and no
// changes to the existing one (even though the second server profile contains
// additional information and is mergeable in principle).
EXPECT_EQ(1U, personal_data_->GetProfiles().size());
EXPECT_EQ(local_profile, *personal_data_->GetProfiles()[0]);
}

TEST_F(PersonalDataManagerTest, RemoveByGUID_ResetsBillingAddress) {
///////////////////////////////////////////////////////////////////////
Expand Down
6 changes: 0 additions & 6 deletions components/autofill/core/browser/webdata/autofill_webdata_backend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@ class AutofillWebDataBackend {
// Autofill records of the database by the sync.
// NOTE: The UI sequence notifications are asynchronous.
virtual void NotifyOnAutofillChangedBySync(syncer::ModelType model_type) = 0;

// Notifies listeners on the UI sequence that conversion of server profiles
// into local profiles is completed.
// NOTE: This method is intended to be called from the DB sequence. The UI
// sequence notifications are asynchronous.
virtual void NotifyOfAddressConversionCompleted() = 0;
};

} // namespace autofill
Expand Down
26 changes: 2 additions & 24 deletions components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#include "components/autofill/core/browser/webdata/autofill_change.h"
#include "components/autofill/core/browser/webdata/autofill_entry.h"
#include "components/autofill/core/browser/webdata/autofill_table.h"
#include "components/autofill/core/browser/webdata/autofill_webdata_backend_util.h"
#include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
#include "components/autofill/core/browser/webdata/autofill_webdata_service_observer.h"
#include "components/autofill/core/common/autofill_clock.h"
Expand Down Expand Up @@ -128,16 +127,13 @@ AutofillWebDataBackendImpl::AutofillWebDataBackendImpl(
scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
scoped_refptr<base::SequencedTaskRunner> db_task_runner,
const base::RepeatingCallback<void(syncer::ModelType)>&
on_autofill_changed_by_sync_callback,
const base::RepeatingClosure& on_address_conversion_completed_callback)
on_autofill_changed_by_sync_callback)
: base::RefCountedDeleteOnSequence<AutofillWebDataBackendImpl>(
std::move(db_task_runner)),
ui_task_runner_(ui_task_runner),
web_database_backend_(web_database_backend),
on_autofill_changed_by_sync_callback_(
on_autofill_changed_by_sync_callback),
on_address_conversion_completed_callback_(
on_address_conversion_completed_callback) {}
on_autofill_changed_by_sync_callback) {}

void AutofillWebDataBackendImpl::AddObserver(
AutofillWebDataServiceObserverOnDBSequence* observer) {
Expand Down Expand Up @@ -219,14 +215,6 @@ void AutofillWebDataBackendImpl::NotifyOnAutofillChangedBySync(
base::BindOnce(on_autofill_changed_by_sync_callback_, model_type));
}

void AutofillWebDataBackendImpl::NotifyOfAddressConversionCompleted() {
DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());

// UI sequence notification.
ui_task_runner_->PostTask(FROM_HERE,
on_address_conversion_completed_callback_);
}

base::SupportsUserData* AutofillWebDataBackendImpl::GetDBUserData() {
DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
if (!user_data_)
Expand Down Expand Up @@ -438,16 +426,6 @@ std::unique_ptr<WDTypedResult> AutofillWebDataBackendImpl::GetServerProfiles(
AUTOFILL_PROFILES_RESULT, std::move(profiles)));
}

WebDatabase::State
AutofillWebDataBackendImpl::ConvertWalletAddressesAndUpdateWalletCards(
const std::string& app_locale,
const std::string& primary_account_email,
WebDatabase* db) {
DCHECK(owning_task_runner()->RunsTasksInCurrentSequence());
return util::ConvertWalletAddressesAndUpdateWalletCards(
app_locale, primary_account_email, this, db);
}

std::unique_ptr<WDTypedResult>
AutofillWebDataBackendImpl::GetCountOfValuesContainedBetween(
const base::Time& begin,
Expand Down
14 changes: 1 addition & 13 deletions components/autofill/core/browser/webdata/autofill_webdata_backend_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ class AutofillWebDataBackendImpl
scoped_refptr<base::SequencedTaskRunner> ui_task_runner,
scoped_refptr<base::SequencedTaskRunner> db_task_runner,
const base::RepeatingCallback<void(syncer::ModelType)>&
on_autofill_changed_by_sync_callback,
const base::RepeatingClosure& on_address_conversion_completed_callback);
on_autofill_changed_by_sync_callback);

AutofillWebDataBackendImpl(const AutofillWebDataBackendImpl&) = delete;
AutofillWebDataBackendImpl& operator=(const AutofillWebDataBackendImpl&) =
Expand All @@ -75,7 +74,6 @@ class AutofillWebDataBackendImpl
const AutofillProfileChange& change) override;
void NotifyOfCreditCardChanged(const CreditCardChange& change) override;
void NotifyOnAutofillChangedBySync(syncer::ModelType model_type) override;
void NotifyOfAddressConversionCompleted() override;
void CommitChanges() override;

// Returns a SupportsUserData object that may be used to store data accessible
Expand Down Expand Up @@ -139,15 +137,6 @@ class AutofillWebDataBackendImpl
WebDatabase* db);
std::unique_ptr<WDTypedResult> GetServerProfiles(WebDatabase* db);

// Converts server profiles to local profiles, comparing profiles using
// |app_locale| and filling in |primary_account_email| into newly converted
// profiles. The task only converts profiles that have not been converted
// before.
WebDatabase::State ConvertWalletAddressesAndUpdateWalletCards(
const std::string& app_locale,
const std::string& primary_account_email,
WebDatabase* db);

// Returns the number of values such that all for autofill entries with that
// value, the interval between creation date and last usage is entirely
// contained between [|begin|, |end|).
Expand Down Expand Up @@ -291,7 +280,6 @@ class AutofillWebDataBackendImpl

base::RepeatingCallback<void(syncer::ModelType)>
on_autofill_changed_by_sync_callback_;
base::RepeatingClosure on_address_conversion_completed_callback_;
base::RepeatingCallback<void(const AutofillProfileDeepChange&)>
on_autofill_profile_changed_cb_;
};
Expand Down

0 comments on commit 712c2a3

Please sign in to comment.