[Autofill Offer] Fix client tag hash missing issue

We found the offer data would be successfully synced down to client but
would not be saved. After debugging we found the client tag hash for the
offer data is missing in the update response. Similar to wallet data,
every time offer data is updated, it will be a full update. So we need
to set this manually.

Bug: 1112095
Change-Id: I304a078f44f5647d4f3da26b7086f0ddec973ab5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441939
Commit-Queue: Siyu An <siyua@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812881}

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

@@ -7,13 +7,13 @@
 #include <utility>
 
 #include "base/logging.h"
-#include "base/strings/string_number_conversions.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
 #include "components/autofill/core/browser/data_model/autofill_offer_data.h"
 #include "components/autofill/core/browser/webdata/autofill_sync_bridge_util.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_backend.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
+#include "components/sync/base/hash_util.h"
 #include "components/sync/model/mutable_data_batch.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
 #include "components/sync/model_impl/sync_metadata_store_change_list.h"
@@ -27,7 +27,7 @@ static int kAutofillWalletOfferSyncBridgeUserDataKey = 0;
 
 std::string GetClientTagFromSpecifics(
     const sync_pb::AutofillOfferSpecifics& specifics) {
-  return base::NumberToString(specifics.id());
+  return syncer::GetUnhashedClientTagFromAutofillOfferSpecifics(specifics);
 }
 
 std::string GetStorageKeyFromSpecifics(

components/sync/base/hash_util.cc

@@ -7,6 +7,7 @@
 #include "base/base64.h"
 #include "base/hash/sha1.h"
 #include "base/notreached.h"
+#include "base/strings/string_number_conversions.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/protocol/sync.pb.h"
 
@@ -46,4 +47,9 @@ std::string GetUnhashedClientTagFromAutofillWalletSpecifics(
   return std::string();
 }
 
+std::string GetUnhashedClientTagFromAutofillOfferSpecifics(
+    const sync_pb::AutofillOfferSpecifics& specifics) {
+  return base::NumberToString(specifics.id());
+}
+
 }  // namespace syncer

components/sync/base/hash_util.h

@@ -8,6 +8,7 @@
 #include <string>
 
 namespace sync_pb {
+class AutofillOfferSpecifics;
 class AutofillWalletSpecifics;
 }  // namespace sync_pb
 
@@ -31,6 +32,12 @@ std::string GenerateSyncableBookmarkHash(
 std::string GetUnhashedClientTagFromAutofillWalletSpecifics(
     const sync_pb::AutofillWalletSpecifics& specifics);
 
+// Helper function to extract client tag from the specifics. For offer data,
+// every time it is synced, it will be a full sync and this client tag is not
+// populated by server.
+std::string GetUnhashedClientTagFromAutofillOfferSpecifics(
+    const sync_pb::AutofillOfferSpecifics& specifics);
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_BASE_HASH_UTIL_H_

components/sync/engine_impl/model_type_worker.cc

@@ -36,20 +36,31 @@ namespace syncer {
 
 namespace {
 
-void AdaptClientTagForWalletData(syncer::EntityData* data) {
-  // Server does not send any client tags for wallet data entities. This code
-  // manually asks the bridge to create the client tags for each entity, so that
-  // we can use ClientTagBasedModelTypeProcessor for WALLET_DATA.
+void AdaptClientTagForFullUpdateData(ModelType model_type,
+                                     syncer::EntityData* data) {
+  // Server does not send any client tags for wallet data entities or offer data
+  // entities. This code manually asks the bridge to create the client tags for
+  // each entity, so that we can use ClientTagBasedModelTypeProcessor for
+  // AUTOFILL_WALLET_DATA or AUTOFILL_WALLET_OFFER.
   if (data->parent_id == "0") {
     // Ignore the permanent root node as that one should have no client tag
     // hash.
     return;
   }
   DCHECK(!data->specifics.has_encrypted());
-  DCHECK(data->specifics.has_autofill_wallet());
-  data->client_tag_hash = ClientTagHash::FromUnhashed(
-      AUTOFILL_WALLET_DATA, GetUnhashedClientTagFromAutofillWalletSpecifics(
-                                data->specifics.autofill_wallet()));
+  if (model_type == AUTOFILL_WALLET_DATA) {
+    DCHECK(data->specifics.has_autofill_wallet());
+    data->client_tag_hash = ClientTagHash::FromUnhashed(
+        AUTOFILL_WALLET_DATA, GetUnhashedClientTagFromAutofillWalletSpecifics(
+                                  data->specifics.autofill_wallet()));
+  } else if (model_type == AUTOFILL_WALLET_OFFER) {
+    DCHECK(data->specifics.has_autofill_offer());
+    data->client_tag_hash = ClientTagHash::FromUnhashed(
+        AUTOFILL_WALLET_OFFER, GetUnhashedClientTagFromAutofillOfferSpecifics(
+                                   data->specifics.autofill_offer()));
+  } else {
+    NOTREACHED();
+  }
 }
 
 }  // namespace
@@ -298,8 +309,9 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
                           specifics_were_encrypted);
     data.is_bookmark_guid_in_specifics_preprocessed =
         AdaptGuidForBookmark(update_entity, &data.specifics);
-  } else if (model_type == AUTOFILL_WALLET_DATA) {
-    AdaptClientTagForWalletData(&data);
+  } else if (model_type == AUTOFILL_WALLET_DATA ||
+             model_type == AUTOFILL_WALLET_OFFER) {
+    AdaptClientTagForFullUpdateData(model_type, &data);
   }
 
   response_data->entity = std::move(data);

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -1645,6 +1645,25 @@ TEST_F(ModelTypeWorkerTest,
   EXPECT_FALSE(response_data.entity.client_tag_hash.value().empty());
 }
 
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataForOfferDataWithMissingClientTagHash) {
+  NormalInitialize();
+  UpdateResponseData response_data;
+
+  // Set up the entity with an arbitrary value for an arbitrary field in the
+  // specifics (so that it _has_ autofill offer specifics).
+  sync_pb::SyncEntity entity;
+  entity.mutable_specifics()->mutable_autofill_offer()->set_id(1234567);
+
+  ASSERT_EQ(ModelTypeWorker::SUCCESS,
+            ModelTypeWorker::PopulateUpdateResponseData(
+                /*cryptographer=*/nullptr, AUTOFILL_WALLET_OFFER, entity,
+                &response_data));
+
+  // The client tag hash gets filled in by the worker.
+  EXPECT_FALSE(response_data.entity.client_tag_hash.value().empty());
+}
+
 class GetLocalChangesRequestTest : public testing::Test {
  public:
   GetLocalChangesRequestTest();