[AF Wallet] Notify remote metadata changes

This CL introduces notification of remote changes for wallet metadata
sync. This is vital for two reasons:
 - PersonalDataManager loads up to date data as soon as it gets changed;
 - It allows to write integration tests for wallet metadata that only
   observe the PDM (and thus test as end-to-end as it gets).

The approach to block self-notification is a bit hacky; it will get
removed in the new USS bridge as this will not listen to MultipleChanged
any more.

Bug: 894001
Change-Id: I1bcdd4b80187c2980c4343bfd37584ad64d93d01
Reviewed-on: https://chromium-review.googlesource.com/c/1309744
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607215}

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc

@@ -284,6 +284,7 @@ template <class DataType>
 bool MergeRemote(
     const syncer::SyncData& remote,
     const base::Callback<bool(const DataType&)>& updater,
+    bool* is_any_local_modified,
     std::unordered_map<std::string, std::unique_ptr<DataType>>* locals,
     syncer::SyncChangeList* changes_to_sync) {
   DCHECK(locals);
@@ -310,8 +311,10 @@ bool MergeRemote(
                       *local_metadata)));
   }
 
-  if (is_local_modified)
+  if (is_local_modified) {
     updater.Run(*local_metadata);
+    *is_any_local_modified = true;
+  }
 
   return true;
 }
@@ -423,6 +426,8 @@ syncer::SyncError AutofillWalletMetadataSyncableService::ProcessSyncChanges(
       base::Bind(&AutofillWalletMetadataSyncableService::UpdateCardStats,
                  base::Unretained(this));
 
+  bool is_any_local_modified = false;
+
   syncer::SyncChangeList changes_to_sync;
   for (const syncer::SyncChange& change : changes_from_sync) {
     const sync_pb::WalletMetadataSpecifics& remote_metadata =
@@ -433,13 +438,13 @@ syncer::SyncError AutofillWalletMetadataSyncableService::ProcessSyncChanges(
       case syncer::SyncChange::ACTION_UPDATE:
         switch (remote_metadata.type()) {
           case sync_pb::WalletMetadataSpecifics::ADDRESS:
-            MergeRemote(change.sync_data(), address_updater, &profiles,
-                        &changes_to_sync);
+            MergeRemote(change.sync_data(), address_updater,
+                        &is_any_local_modified, &profiles, &changes_to_sync);
             break;
 
           case sync_pb::WalletMetadataSpecifics::CARD:
-            MergeRemote(change.sync_data(), card_updater, &cards,
-                        &changes_to_sync);
+            MergeRemote(change.sync_data(), card_updater,
+                        &is_any_local_modified, &cards, &changes_to_sync);
             break;
 
           case sync_pb::WalletMetadataSpecifics::UNKNOWN:
@@ -478,6 +483,15 @@ syncer::SyncError AutofillWalletMetadataSyncableService::ProcessSyncChanges(
   syncer::SyncError status;
   if (!changes_to_sync.empty())
     status = SendChangesToSyncServer(changes_to_sync);
+  if (is_any_local_modified) {
+    // TODO(crbug.com/900607): Remove the need to listen to
+    // AutofillMultipleChanged() in the new USS implementation so that we can
+    // get rid of this hack.
+    DCHECK(!ignore_multiple_changed_notification_);
+    ignore_multiple_changed_notification_ = true;
+    web_data_backend_->NotifyOfMultipleAutofillChanges();
+    ignore_multiple_changed_notification_ = false;
+  }
 
   return status;
 }
@@ -529,6 +543,13 @@ void AutofillWalletMetadataSyncableService::CreditCardChanged(
 }
 
 void AutofillWalletMetadataSyncableService::AutofillMultipleChanged() {
+  if (ignore_multiple_changed_notification_) {
+    // TODO(crbug.com/900607): Remove the need to listen to
+    // AutofillMultipleChanged() in the new USS implementation so that we can
+    // get rid of this hack.
+    return;
+  }
+
   if (sync_processor_ && track_wallet_data_)
     MergeData(cache_);
 }
@@ -559,6 +580,7 @@ AutofillWalletMetadataSyncableService::AutofillWalletMetadataSyncableService(
     : web_data_backend_(web_data_backend),
       scoped_observer_(this),
       track_wallet_data_(false),
+      ignore_multiple_changed_notification_(false),
       weak_ptr_factory_(this) {
   scoped_observer_.Add(web_data_backend_);
 }
@@ -629,21 +651,24 @@ syncer::SyncMergeResult AutofillWalletMetadataSyncableService::MergeData(
       base::Bind(&AutofillWalletMetadataSyncableService::UpdateCardStats,
                  base::Unretained(this));
 
+  bool is_any_local_modified = false;
+
   syncer::SyncChangeList changes_to_sync;
   for (const syncer::SyncData& remote : sync_data) {
     DCHECK(remote.IsValid());
     DCHECK_EQ(syncer::AUTOFILL_WALLET_METADATA, remote.GetDataType());
     switch (remote.GetSpecifics().wallet_metadata().type()) {
       case sync_pb::WalletMetadataSpecifics::ADDRESS:
-        if (!MergeRemote(remote, address_updater, &profiles,
-                         &changes_to_sync)) {
+        if (!MergeRemote(remote, address_updater, &is_any_local_modified,
+                         &profiles, &changes_to_sync)) {
           changes_to_sync.push_back(syncer::SyncChange(
               FROM_HERE, syncer::SyncChange::ACTION_DELETE, remote));
         }
         break;
 
       case sync_pb::WalletMetadataSpecifics::CARD:
-        if (!MergeRemote(remote, card_updater, &cards, &changes_to_sync)) {
+        if (!MergeRemote(remote, card_updater, &is_any_local_modified, &cards,
+                         &changes_to_sync)) {
           changes_to_sync.push_back(syncer::SyncChange(
               FROM_HERE, syncer::SyncChange::ACTION_DELETE, remote));
         }
@@ -680,6 +705,15 @@ syncer::SyncMergeResult AutofillWalletMetadataSyncableService::MergeData(
 
   if (!changes_to_sync.empty())
     result.set_error(SendChangesToSyncServer(changes_to_sync));
+  if (is_any_local_modified) {
+    // TODO(crbug.com/900607): Remove the need to listen to
+    // AutofillMultipleChanged() in the new USS implementation so that we can
+    // get rid of this hack.
+    DCHECK(!ignore_multiple_changed_notification_);
+    ignore_multiple_changed_notification_ = true;
+    web_data_backend_->NotifyOfMultipleAutofillChanges();
+    ignore_multiple_changed_notification_ = false;
+  }
 
   return result;
 }

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.h

@@ -154,6 +154,11 @@ class AutofillWalletMetadataSyncableService
   // data entry.
   bool track_wallet_data_;
 
+  // Indicates that we should ignore multiple changed notification. This is used
+  // to block reflection and not to act on notification that we've triggered
+  // ourselves.
+  bool ignore_multiple_changed_notification_;
+
   base::WeakPtrFactory<AutofillWalletMetadataSyncableService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AutofillWalletMetadataSyncableService);

components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc

@@ -1301,5 +1301,37 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest, NewLocalCard) {
   MergeMetadata(&local_, &remote_);
 }
 
+// Tests that processing remote changes does not trigger a merge. This is a
+// specific test for reflection blocking in notifications.
+TEST_F(AutofillWalletMetadataSyncableServiceTest,
+       RemoteChangesDoNotTriggerMerge) {
+  // Make the backend broadcast back the notifications it receives
+  ON_CALL(backend_, NotifyOfMultipleAutofillChanges())
+      .WillByDefault(
+          DoAll(Invoke(&local_, &MockService::AutofillMultipleChanged),
+                Invoke(&remote_, &MockService::AutofillMultipleChanged)));
+
+  // Get initial data from |remote_| into |local_|.
+  local_.UpdateAddressStats(BuildAddress(kAddr1, 2, 2, true));
+  local_.UpdateCardStats(BuildCard(kCard1, 5, 6, kAddr1));
+  remote_.UpdateAddressStats(BuildAddress(kAddr1, 2, 2, true));
+  remote_.UpdateCardStats(BuildCard(kCard1, 5, 6, kAddr1));
+  MergeMetadata(&local_, &remote_);
+
+  // Silently update local card in the DB.
+  local_.UpdateCardStats(BuildCard(kCard1, 7, 8, kAddr1));
+
+  // Receive a remote update of the address.
+  syncer::SyncChangeList changes;
+  changes.push_back(BuildAddressChange(
+      syncer::SyncChange::ACTION_UPDATE, kAddr1SyncTag,
+      sync_pb::WalletMetadataSpecifics::ADDRESS, kAddr1Utf8, 10, 20, true));
+
+  // Processing remote change should not trigger a commit of the local change.
+  EXPECT_CALL(local_, SendChangesToSyncServer(_)).Times(0);
+
+  local_.ProcessSyncChanges(FROM_HERE, changes);
+}
+
 }  // namespace
 }  // namespace autofill