Skip to content

Commit

Permalink
ProfileTokenQuality: Persist new observations
Browse files Browse the repository at this point in the history
crrev.com/c/4720559 added the ability to store observations in
AutofillTable. This CL adds the corresponding UpdateProfile() call
via the PDM whenever new observations are collected on form submit.

The PDM rejects updates if the updated profile
EqualsForUpdatePurposes() with the existing profile that the PDM
is aware of. As such, this function is updated to take observations into
consideration.

The updates caused by this logic won't modify the modification date of
the updated profile, since AutofillProfile::operator== doesn't consider
changes in the observations. See:
https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/personal_data_manager.cc;l=2588-2592;drc=cdd964007eb01c64477e65df08f088b500ed229d

Bug: 1453650
Change-Id: Iacedd7798b9c59c3731018a81741e87f11f58785
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4984032
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Christoph Schwering <schwering@google.com>
Commit-Queue: Florian Leimgruber <fleimgruber@google.com>
Cr-Commit-Position: refs/heads/main@{#1216872}
  • Loading branch information
florianleimgruber authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent 18b0048 commit 88aa86f
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3690,8 +3690,10 @@ TEST_F(BrowserAutofillManagerTest, FillAddressForm_CollectObservations) {
.empty();
}));

// Submit the form and expect observations for all of the form's types.
// Submit the form and expect observations for all of the form's types. This
// updates the `profile` in `personal_data()`, invalidating the pointer.
FormSubmitted(filled_form);
profile = personal_data().GetProfileByGUID(kElvisProfileGuid);
EXPECT_TRUE(base::ranges::none_of(
*form_structure, [&](const std::unique_ptr<AutofillField>& field) {
return profile->token_quality()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,7 @@ bool AutofillProfile::EqualsForUpdatePurposes(
return use_count() == new_profile.use_count() &&
UseDateEqualsInSeconds(&new_profile) &&
language_code() == new_profile.language_code() &&
token_quality() == new_profile.token_quality() &&
Compare(new_profile) == 0;
}

Expand Down
30 changes: 30 additions & 0 deletions components/autofill/core/browser/personal_data_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,36 @@ TEST_F(PersonalDataManagerTest, AddUpdateRemoveProfiles) {
ExpectSameElements(profiles, personal_data_->GetProfiles());
}

// Tests that `UpdateProfile()` takes changes in the `ProfileTokenQuality`
// observations into considerations.
TEST_F(PersonalDataManagerTest, UpdateProfile_NewObservations) {
base::test::ScopedFeatureList feature{
features::kAutofillTrackProfileTokenQuality};

// Add a profile without observations at `kArbitraryTime`.
TestAutofillClock test_clock;
test_clock.SetNow(kArbitraryTime);
AutofillProfile profile = test::GetFullProfile();
AddProfileToPersonalDataManager(profile);
test_clock.SetNow(kSomeLaterTime);

// Add an observation, as might happen during a form submit.
test_api(profile.token_quality())
.AddObservation(NAME_FIRST,
ProfileTokenQuality::ObservationType::kAccepted);
UpdateProfileOnPersonalDataManager(profile);

// Expect that `UpdateProfile()` didn't reject the update as a no-op.
// Since new observations are considered a metadata change, further expected
// that the modification date hasn't changed.
const AutofillProfile* pdm_profile =
personal_data_->GetProfileByGUID(profile.guid());
EXPECT_THAT(
pdm_profile->token_quality().GetObservationTypesForFieldType(NAME_FIRST),
UnorderedElementsAre(ProfileTokenQuality::ObservationType::kAccepted));
EXPECT_EQ(profile.modification_date(), kArbitraryTime);
}

// Tests that when the value for a type changes, `UpdateProfile()` resets the
// observations for that type.
TEST_F(PersonalDataManagerTest, UpdateProfile_ResetObservations) {
Expand Down
33 changes: 31 additions & 2 deletions components/autofill/core/browser/profile_token_quality.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,26 @@ ProfileTokenQuality::ProfileTokenQuality(const ProfileTokenQuality& other) =

ProfileTokenQuality::~ProfileTokenQuality() = default;

bool ProfileTokenQuality::operator==(const ProfileTokenQuality& other) const {
if (profile_->guid() != other.profile_->guid() ||
observations_.size() != other.observations_.size()) {
return false;
}
// Element-wise comparison between `observations_` and `other.observations_`.
// base::circular_deque<> intentionally doesn't define a comparison operator.
using map_entry_t =
std::pair<ServerFieldType, base::circular_deque<Observation>>;
return base::ranges::equal(observations_, other.observations_,
[](const map_entry_t& a, const map_entry_t& b) {
return a.first == b.first &&
base::ranges::equal(a.second, b.second);
});
}

bool ProfileTokenQuality::operator!=(const ProfileTokenQuality& other) const {
return !operator==(other);
}

// static
bool ProfileTokenQuality::IsStoredType(ServerFieldType type) {
return base::Contains(AutofillTable::GetStoredTypesForAutofillProfile(),
Expand Down Expand Up @@ -209,8 +229,7 @@ void ProfileTokenQuality::SaveObservationsForFilledFormForAllSubmittedProfiles(
pdm.GetProfileByGUID(*field->autofill_source_profile_guid());
if (profile && profile->token_quality().AddObservationsForFilledForm(
form_structure, form_data, pdm)) {
// TODO(crbug.com/1453650): Update `*profile` in the database, once
// `AutofillTable` supports storing token quality.
pdm.UpdateProfile(*profile);
}
}
}
Expand Down Expand Up @@ -355,6 +374,16 @@ void ProfileTokenQuality::ResetObservationsForDifferingTokens(
}
}

bool ProfileTokenQuality::Observation::operator==(
const ProfileTokenQuality::Observation& other) const {
return type == other.type && form_hash == other.form_hash;
}

bool ProfileTokenQuality::Observation::operator!=(
const ProfileTokenQuality::Observation& other) const {
return !operator==(other);
}

ProfileTokenQuality::FormSignatureHash
ProfileTokenQuality::GetFormSignatureHash(FormSignature form_signature) const {
// Just take the lowest 8 bits of the `form_signature`.
Expand Down
6 changes: 6 additions & 0 deletions components/autofill/core/browser/profile_token_quality.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ class ProfileTokenQuality {
ProfileTokenQuality(const ProfileTokenQuality& other);
~ProfileTokenQuality();

bool operator==(const ProfileTokenQuality& other) const;
bool operator!=(const ProfileTokenQuality& other) const;

// Determines if a `type` is considered stored. Observations are only tracked
// for stored types.
static bool IsStoredType(ServerFieldType type);
Expand Down Expand Up @@ -209,6 +212,9 @@ class ProfileTokenQuality {
// Getters expose unknown values as `kUnknown`.
std::underlying_type_t<ObservationType> type;
FormSignatureHash form_hash = FormSignatureHash(0);

bool operator==(const Observation& other) const;
bool operator!=(const Observation& other) const;
};

// Returns a low-entry hash of the `form_signature`.
Expand Down

0 comments on commit 88aa86f

Please sign in to comment.