[Autofill] Sync new wallet metadata fields through Chrome Sync.

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

Index: components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc
diff --git a/components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc b/components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc
index 57dabb744cba4af43f925fa209661ee0df6c3f1e..bd6ee55cbcf9aff52dd62e1191267a6983c353ea 100644
--- a/components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc
+++ b/components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service.cc
@@ -40,29 +40,42 @@ void* UserDataKey() {
   return reinterpret_cast<void*>(&user_data_key);
 }
 
-// Returns syncable metadata for the |local| profile or credit card.
+// Sets the basic syncable |metadata| for the |local_data_model|.

[please use gerrit instead, 2017/01/16 18:20:12]

s/SetBasicData/SetCommonMetadata

[sebsg, 2017/01/16 18:57:39]

Done.

+void SetBasicData(sync_pb::WalletMetadataSpecifics::Type type,
+                  const std::string& server_id,
+                  const AutofillDataModel& local_data_model,
+                  sync_pb::WalletMetadataSpecifics* metadata) {
+  metadata->set_type(type);
+  metadata->set_id(server_id);
+  metadata->set_use_count(local_data_model.use_count());
+  metadata->set_use_date(local_data_model.use_date().ToInternalValue());
+}
+
+// Returns syncable metadata for the |local_profile|.
 syncer::SyncData BuildSyncData(sync_pb::WalletMetadataSpecifics::Type type,
                                const std::string& server_id,
-                               const AutofillDataModel& local) {
+                               const AutofillProfile& local_profile) {
   sync_pb::EntitySpecifics entity;
   sync_pb::WalletMetadataSpecifics* metadata = entity.mutable_wallet_metadata();
-  metadata->set_type(type);
-  metadata->set_id(server_id);
-  metadata->set_use_count(local.use_count());
-  metadata->set_use_date(local.use_date().ToInternalValue());
-
-  std::string sync_tag;
-  switch (type) {
-    case sync_pb::WalletMetadataSpecifics::ADDRESS:
-      sync_tag = "address-" + server_id;
-      break;
-    case sync_pb::WalletMetadataSpecifics::CARD:
-      sync_tag = "card-" + server_id;
-      break;
-    case sync_pb::WalletMetadataSpecifics::UNKNOWN:
-      NOTREACHED();
-      break;
-  }
+  SetBasicData(type, server_id, local_profile, metadata);
+  metadata->set_address_has_converted(local_profile.has_converted());
+  std::string sync_tag = "address-" + server_id;
+
+  return syncer::SyncData::CreateLocalData(sync_tag, sync_tag, entity);
+}
+
+// Returns syncable metadata for the |local_card|.
+syncer::SyncData BuildSyncData(sync_pb::WalletMetadataSpecifics::Type type,
+                               const std::string& server_id,
+                               const CreditCard& local_card) {
+  sync_pb::EntitySpecifics entity;
+  sync_pb::WalletMetadataSpecifics* metadata = entity.mutable_wallet_metadata();
+  SetBasicData(type, server_id, local_card, metadata);
+  // The strings must be in valid UTF-8 to sync.

[please use gerrit instead, 2017/01/16 18:20:12]

I can't think of a situation when this string would not be utf8. If neither can
you, then DCHECK() for utf-ness instead of re-encoding the data.

[sebsg, 2017/01/16 18:57:39]

In the tests, non UTF-8 values are used as ids for the addresses and cards, so
if I remove this the tests will crash. I'll ask zea@ if this is still relevant
and remove it if it's not.

[please use gerrit instead, 2017/01/17 14:50:04]

Ah, you're right. Server ID is not guaranteed to be unicode. Only local IDs are
unicode.

+  std::string billing_address_id;
+  base::Base64Encode(local_card.billing_address_id(), &billing_address_id);
+  metadata->set_card_billing_address_id(billing_address_id);
+  std::string sync_tag = "card-" + server_id;
 
   return syncer::SyncData::CreateLocalData(sync_tag, sync_tag, entity);
 }
@@ -142,6 +155,26 @@ void ApplyChangesToCache(const syncer::SyncChangeList& changes,
   }
 }
 
+// Updates the has_converted field of the |local_profile| to the value found in
+// the remote metadata.
+void UpdateSpecificLocalMetadata(
+    const sync_pb::WalletMetadataSpecifics& remote_metadata,
+    AutofillProfile* local_profile) {
+  local_profile->set_has_converted(remote_metadata.address_has_converted());
+}
+
+// Updates the billing_address_id field of the |local_card| to the value found
+// in the remote metadata.
+void UpdateSpecificLocalMetadata(
+    const sync_pb::WalletMetadataSpecifics& remote_metadata,
+    CreditCard* local_card) {
+  // The sync data is in UTF-8, decode it before saving.
+  std::string billing_address_id;
+  base::Base64Decode(remote_metadata.card_billing_address_id(),
+                     &billing_address_id);
+  local_card->set_billing_address_id(billing_address_id);
+}
+
 // Merges |remote| metadata into a collection of metadata |locals|. Returns true
 // if the corresponding local metadata was found.
 //
@@ -165,21 +198,17 @@ bool MergeRemote(
   std::unique_ptr<DataType> local_metadata = std::move(it->second);
   locals->erase(it);
 
-  size_t remote_use_count =

[please use gerrit instead, 2017/01/16 18:20:12]

You appear to have wiped out the comparison of remote use count to local use
count. Any good reason for that?

[sebsg, 2017/01/16 18:57:39]

Before, the value of each individual field was checked and the bigger number was
kept. However, with the addition of the new fields this is no longer possible,
because for guids and bools, the is no "bigger" version. Therefore I opted to
keep the values of the data model with the most recent use_date.

Like I mentioned in the CL description, I'm still unsure about this. I'm
thinking of checking the has_converted first and always keep an address that was
converted over and address that was not. If the values are the same, then
fallback to the most recent use_date. WDYT?

[please use gerrit instead, 2017/01/17 14:47:37]

You should keep the "max" of every field. The "max" of "has_converted" is
"true", because it starts from "false" and eventually switches to "true". It
should never go back.

With your current logic, the following flow does not work correctly:

Computer A:
1) User sets the billing address of card.
2) Server address gets converted to local address.
3) User uses the card twice on 2017/01/01.

Computer B:
1) Before sync happens, user uses the same card once on 2017/01/02.
2) Sync happens.

With your code, the billing address and "has_converted" would be wiped, because
Computer B has later use_date and did not yet convert the address or set the
billing address of the card. Also, the use count would drop from 2 to 1.

Therefore, the best approach is to check every field. You can use the use_date
as the supplement for resolving conflicts between different billing address
identifiers.

-      base::checked_cast<size_t>(remote_metadata.use_count());
   bool is_local_modified = false;
   bool is_remote_outdated = false;
-  if (local_metadata->use_count() < remote_use_count) {
-    local_metadata->set_use_count(remote_use_count);
-    is_local_modified = true;
-  } else if (local_metadata->use_count() > remote_use_count) {
-    is_remote_outdated = true;
-  }
 
+  // Keep the most recent information.
   base::Time remote_use_date =
       base::Time::FromInternalValue(remote_metadata.use_date());
   if (local_metadata->use_date() < remote_use_date) {
     local_metadata->set_use_date(remote_use_date);
+    local_metadata->set_use_count(
+        base::checked_cast<size_t>(remote_metadata.use_count()));
+    UpdateSpecificLocalMetadata(remote_metadata, local_metadata.get());
     is_local_modified = true;
   } else if (local_metadata->use_date() > remote_use_date) {
     is_remote_outdated = true;
@@ -517,18 +546,43 @@ syncer::SyncMergeResult AutofillWalletMetadataSyncableService::MergeData(
 void AutofillWalletMetadataSyncableService::AutofillDataModelChanged(
     const std::string& server_id,
     const sync_pb::WalletMetadataSpecifics::Type& type,
-    const AutofillDataModel& local) {
+    const AutofillProfile& local) {
   auto it = FindServerIdAndTypeInCache(server_id, type, &cache_);
   if (it == cache_.end())
     return;
 
   const sync_pb::WalletMetadataSpecifics& remote =
       it->GetSpecifics().wallet_metadata();
+
+  SendUpdateIfMoreRecent(
+      type, local, BuildSyncData(remote.type(), server_id, local), remote);
+}
+
+void AutofillWalletMetadataSyncableService::AutofillDataModelChanged(
+    const std::string& server_id,
+    const sync_pb::WalletMetadataSpecifics::Type& type,
+    const CreditCard& local) {
+  auto it = FindServerIdAndTypeInCache(server_id, type, &cache_);
+  if (it == cache_.end())
+    return;
+
+  const sync_pb::WalletMetadataSpecifics& remote =
+      it->GetSpecifics().wallet_metadata();
+
+  SendUpdateIfMoreRecent(
+      type, local, BuildSyncData(remote.type(), server_id, local), remote);
+}
+
+void AutofillWalletMetadataSyncableService::SendUpdateIfMoreRecent(
+    const sync_pb::WalletMetadataSpecifics::Type& type,
+    const AutofillDataModel& local,
+    const syncer::SyncData& sync_data,
+    const sync_pb::WalletMetadataSpecifics& remote) {
   if (base::checked_cast<size_t>(remote.use_count()) < local.use_count() &&
       base::Time::FromInternalValue(remote.use_date()) < local.use_date()) {
     SendChangesToSyncServer(syncer::SyncChangeList(
         1, syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
-                              BuildSyncData(remote.type(), server_id, local))));
+                              sync_data)));
   }
 }