Commit c20eb6ac authored by Siyu An's avatar Siyu An Committed by Commit Bot

[Autofill Offer] Fixed missing notification when offer data changed

The issue is the offer data is only updated when relaunch (or when
personal_data_manager::refresh() is invoked) and not updated when
new offer reaches client.

The reason is:

The full sync flow should be:
1. Offer sync bridge receive the data and populates to autofill table
if found difference.
2. Notify UI sequence (personal data manager) about the change.
3. Personal data manager sends query to autofill webdata service to
get data from autofill table.
4. Personal data manager gets notified by OnWebDataServiceRequestDone
and refreshes local cache.
5. Autofill Offer Manager being a personal data manager observer
gets notified.
6. Autofill Offer manager updates eligible domain calling
UpdateEligibleMerchantDomains.

Previously I thought the OnWebDataServiceRequestDone would be
automatically invoked when the sync is finished, so we miss step 2
and 3. Here in this CL fix it.

Bug: 1112095
Change-Id: I38ac61f3d5608eec1c8ac92538e457e566b3f538
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2444454Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Commit-Queue: Siyu An <siyua@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814321}
parent a245cdbf
...@@ -190,8 +190,10 @@ void AutofillWalletOfferSyncBridge::MergeRemoteData( ...@@ -190,8 +190,10 @@ void AutofillWalletOfferSyncBridge::MergeRemoteData(
std::vector<std::unique_ptr<AutofillOfferData>> existing_offers; std::vector<std::unique_ptr<AutofillOfferData>> existing_offers;
table->GetCreditCardOffers(&existing_offers); table->GetCreditCardOffers(&existing_offers);
if (AreAnyItemsDifferent(existing_offers, offer_data)) bool offer_data_changed = AreAnyItemsDifferent(existing_offers, offer_data);
if (offer_data_changed) {
table->SetCreditCardOffers(offer_data); table->SetCreditCardOffers(offer_data);
}
// Commit the transaction to make sure the data and the metadata with the // Commit the transaction to make sure the data and the metadata with the
// new progress marker is written down (especially on Android where we // new progress marker is written down (especially on Android where we
...@@ -199,6 +201,11 @@ void AutofillWalletOfferSyncBridge::MergeRemoteData( ...@@ -199,6 +201,11 @@ void AutofillWalletOfferSyncBridge::MergeRemoteData(
// even if the wallet data has not changed because the model type state incl. // even if the wallet data has not changed because the model type state incl.
// the progress marker always changes. // the progress marker always changes.
web_data_backend_->CommitChanges(); web_data_backend_->CommitChanges();
if (offer_data_changed) {
// TODO(crbug.com/1112095): Add enum to indicate what actually changed.
web_data_backend_->NotifyOfMultipleAutofillChanges();
}
} }
AutofillTable* AutofillWalletOfferSyncBridge::GetAutofillTable() { AutofillTable* AutofillWalletOfferSyncBridge::GetAutofillTable() {
......
...@@ -244,6 +244,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_NewData) { ...@@ -244,6 +244,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_NewData) {
&offer_specifics); &offer_specifics);
EXPECT_CALL(*backend(), CommitChanges()); EXPECT_CALL(*backend(), CommitChanges());
EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
StartSyncing({offer_specifics}); StartSyncing({offer_specifics});
// Only the server offer should be present on the client. // Only the server offer should be present on the client.
...@@ -259,6 +260,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_NoData) { ...@@ -259,6 +260,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_NoData) {
table()->SetCreditCardOffers({client_data}); table()->SetCreditCardOffers({client_data});
EXPECT_CALL(*backend(), CommitChanges()); EXPECT_CALL(*backend(), CommitChanges());
EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
StartSyncing({}); StartSyncing({});
EXPECT_TRUE(GetAllLocalData().empty()); EXPECT_TRUE(GetAllLocalData().empty());
...@@ -275,6 +277,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_LogDataValidity) { ...@@ -275,6 +277,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, MergeSyncData_LogDataValidity) {
offer_specifics2.clear_id(); offer_specifics2.clear_id();
EXPECT_CALL(*backend(), CommitChanges()); EXPECT_CALL(*backend(), CommitChanges());
EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
StartSyncing({offer_specifics1, offer_specifics2}); StartSyncing({offer_specifics1, offer_specifics2});
...@@ -292,6 +295,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, ApplyStopSyncChanges_ClearAllData) { ...@@ -292,6 +295,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, ApplyStopSyncChanges_ClearAllData) {
table()->SetCreditCardOffers({client_data}); table()->SetCreditCardOffers({client_data});
EXPECT_CALL(*backend(), CommitChanges()); EXPECT_CALL(*backend(), CommitChanges());
EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
// Passing in a non-null metadata change list indicates to the bridge that // Passing in a non-null metadata change list indicates to the bridge that
// sync is stopping but the data type is not disabled. // sync is stopping but the data type is not disabled.
...@@ -311,6 +315,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, ApplyStopSyncChanges_KeepAllData) { ...@@ -311,6 +315,7 @@ TEST_F(AutofillWalletOfferSyncBridgeTest, ApplyStopSyncChanges_KeepAllData) {
// We do not write to DB at all, so we should not commit any changes. // We do not write to DB at all, so we should not commit any changes.
EXPECT_CALL(*backend(), CommitChanges()).Times(0); EXPECT_CALL(*backend(), CommitChanges()).Times(0);
EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges()).Times(0);
// Passing in a null metadata change list indicates to the bridge that // Passing in a null metadata change list indicates to the bridge that
// sync is stopping and the data type is disabled. // sync is stopping and the data type is disabled.
......
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