[Sync] ApplySyncChanges autofill implementation.

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

Index: components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
diff --git a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc b/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
index 5b649892599217e67fadfbfac4a04e9729ecfce9..2fc791497c2f3a1e916e072ce01890cf0729f4e9 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
@@ -4,6 +4,8 @@
 
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
 
+#include <algorithm>
+#include <set>
 #include <unordered_set>
 #include <utility>
 #include <vector>
@@ -21,6 +23,18 @@
 #include "components/sync/model/mutable_data_batch.h"
 #include "net/base/escape.h"
 
+using base::Optional;
+using base::Time;
+using sync_pb::AutofillSpecifics;
+using syncer::EntityChange;
+using syncer::EntityChangeList;
+using syncer::EntityData;
+using syncer::EntityDataMap;
+using syncer::MetadataChangeList;
+using syncer::ModelError;
+using syncer::ModelTypeChangeProcessor;
+using syncer::MutableDataBatch;
+
 namespace autofill {
 
 namespace {
@@ -28,6 +42,12 @@ namespace {
 const char kAutocompleteEntryNamespaceTag[] = "autofill_entry|";
 const char kAutocompleteTagDelimiter[] = "|";
 
+// Simplify checking for optional errors and returning only when present.
+#define RETURN_IF(x)      \

[maxbogue, 2017/01/12 00:15:16]

Reading through where this is used, I think I'd prefer it to be RETURN_IF_ERROR.

[skym, 2017/01/12 17:44:00]

Done.

+  if (auto ret_val = x) { \
+    return ret_val;       \
+  }
+
 void* UserDataKey() {
   // Use the address of a static that COMDAT folding won't ever collide
   // with something else.
@@ -35,12 +55,10 @@ void* UserDataKey() {
   return reinterpret_cast<void*>(&user_data_key);
 }
 
-std::unique_ptr<syncer::EntityData> CreateEntityData(
-    const AutofillEntry& entry) {
-  auto entity_data = base::MakeUnique<syncer::EntityData>();
+std::unique_ptr<EntityData> CreateEntityData(const AutofillEntry& entry) {
+  auto entity_data = base::MakeUnique<EntityData>();
   entity_data->non_unique_name = base::UTF16ToUTF8(entry.key().name());
-  sync_pb::AutofillSpecifics* autofill =
-      entity_data->specifics.mutable_autofill();
+  AutofillSpecifics* autofill = entity_data->specifics.mutable_autofill();
   autofill->set_name(base::UTF16ToUTF8(entry.key().name()));
   autofill->set_value(base::UTF16ToUTF8(entry.key().value()));
   autofill->add_usage_timestamp(entry.date_created().ToInternalValue());
@@ -62,6 +80,191 @@ std::string GetStorageKeyFromModel(const AutofillKey& key) {
                                    base::UTF16ToUTF8(key.value()));
 }
 
+AutofillEntry MergeEntryDates(const AutofillEntry& first,

[maxbogue, 2017/01/12 00:15:16]

Honestly variable names like a and b would probably be just as clear here. Or
entry1 and entry2 if you want to make them more descriptive.

[skym, 2017/01/12 17:44:00]

Done.

+                              const AutofillEntry& second) {
+  DCHECK(first.key() == second.key());
+  return AutofillEntry(
+      first.key(), std::min(first.date_created(), second.date_created()),
+      std::max(first.date_last_used(), second.date_last_used()));
+}
+
+bool ParseStorageKey(const std::string& storage_key, AutofillKey* out_key) {
+  AutofillSyncStorageKey proto;
+  if (proto.ParseFromString(storage_key)) {
+    *out_key = AutofillKey(base::UTF8ToUTF16(proto.name()),
+                           base::UTF8ToUTF16((proto.value())));
+    return true;
+  } else {
+    return false;
+  }
+}
+
+AutofillEntry CreateAutofillEntry(const AutofillSpecifics& autofill_specifics) {
+  AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()),
+                  base::UTF8ToUTF16(autofill_specifics.value()));
+  Time date_created, date_last_used;
+  const google::protobuf::RepeatedField<int64_t>& timestamps =
+      autofill_specifics.usage_timestamp();
+  if (!timestamps.empty()) {
+    std::vector<int64_t> sorted(timestamps.begin(), timestamps.end());

[maxbogue, 2017/01/12 00:15:16]

Consider using std::minmax_element
(http://en.cppreference.com/w/cpp/algorithm/minmax_element) to get the min and
max elements without needing to construct a separate list or sort it.

[skym, 2017/01/12 17:44:00]

Love it! Done.

+    std::sort(sorted.begin(), sorted.end());
+    date_created = Time::FromInternalValue(*sorted.begin());
+    date_last_used = Time::FromInternalValue(*sorted.rbegin());
+  }
+  return AutofillEntry(key, date_created, date_last_used);
+}
+
+// This is used to respond to ApplySyncChanges() and MergeSyncData(). Attempts
+// to lazily load local data, and then react to sync data by maintain internal
+// state until flush calls are made, at which point the applicable modifications
+// should be sent towards local and sync directions.
+class SyncDifferenceTracker {
+ public:
+  explicit SyncDifferenceTracker(AutofillTable* table) : table_(table) {}
+
+  Optional<ModelError> IncorporateRemoteSpecifics(
+      const std::string& storage_key,
+      const AutofillSpecifics& specifics) {
+    if (!specifics.has_value()) {
+      // A long time ago autofill had a different format, and it's possible we
+      // could encounter some of that legacy data. It is not useful to us,
+      // because an autofill entry with no value will not place any text in a
+      // form for the user. So drop all of these on the floor.
+      DVLOG(1) << "Dropping old-style autofill profile change.";
+      return {};
+    }
+
+    const AutofillEntry remote = CreateAutofillEntry(specifics);
+    DCHECK_EQ(storage_key, GetStorageKeyFromModel(remote.key()));
+
+    Optional<AutofillEntry> local;
+    if (!ReadEntry(remote.key(), &local)) {
+      return ModelError(FROM_HERE, "Failed reading from WebDatabase.");
+    } else if (!local) {
+      save_to_local_.push_back(remote);
+    } else if (remote != local.value()) {
+      if (specifics.usage_timestamp().empty()) {
+        // Skip merging if there are no timestamps. We don't want to wipe out
+        // a local value of |date_created| if the remote copy is oddly formed.
+        save_to_sync_.push_back(local.value());
+      } else {
+        const AutofillEntry merged = MergeEntryDates(local.value(), remote);
+        save_to_local_.push_back(merged);
+        save_to_sync_.push_back(merged);
+      }
+      unique_to_local_.erase(local.value());
+    }
+    return {};
+  }
+
+  Optional<ModelError> IncorporateRemoteDelete(const std::string& storage_key) {
+    AutofillKey key;
+    if (!ParseStorageKey(storage_key, &key)) {
+      return ModelError(FROM_HERE, "Failed parsing storage key.");
+    }
+    delete_from_local_.insert(key);
+    return {};
+  }
+
+  Optional<ModelError> FlushToLocal(AutofillWebDataBackend* web_data_backend) {
+    for (const AutofillKey& key : delete_from_local_) {
+      if (!table_->RemoveFormElement(key.name(), key.value())) {
+        return ModelError(FROM_HERE, "Failed deleting from WebDatabase");
+      }
+    }
+    if (!table_->UpdateAutofillEntries(save_to_local_)) {
+      return ModelError(FROM_HERE, "Failed updating WebDatabase");
+    }
+    if (!delete_from_local_.empty() || !save_to_local_.empty()) {
+      web_data_backend->NotifyOfMultipleAutofillChanges();
+    }
+    return {};
+  }
+
+  Optional<ModelError> FlushToSync(
+      bool include_local_only,
+      std::unique_ptr<MetadataChangeList> metadata_change_list,
+      ModelTypeChangeProcessor* change_processor) {
+    for (const AutofillEntry& entry : save_to_sync_) {
+      change_processor->Put(GetStorageKeyFromModel(entry.key()),
+                            CreateEntityData(entry),
+                            metadata_change_list.get());
+    }
+    if (include_local_only) {
+      if (!InitializeIfNeeded()) {
+        return ModelError(FROM_HERE, "Failed reading from WebDatabase.");
+      }
+      for (const AutofillEntry& entry : unique_to_local_) {
+        // This should never be true because only ApplySyncChanges should be

[maxbogue, 2017/01/12 00:15:16]

I don't believe this is true; deletes can come through merge
(https://cs.chromium.org/chromium/src/components/sync/model/entity_data.h?l=73).

[skym, 2017/01/12 17:44:00]

Sigh... Yeah, I messed this up (great catch btw Max). Didn't realize we had to
deal with deletes during merge. I think all the existing USS merge
implementations to date have messed this up.

https://cs.chromium.org/chromium/src/components/sync/device_info/device_info_...
https://cs.chromium.org/chromium/src/components/reading_list/ios/reading_list...
https://codereview.chromium.org/2432803003/diff/120001/chrome/browser/chromeo...

How did the directory merge work anyways? We should investigate. It makes sense
from the perspective that a client might be repeatedly enabling-disabling sync
to accept tombstones. But I'm not sure it makes sense from a first connection
standpoint to delete things that happen to match.

Created crbug.com/680550, added a TODO to the merge function. I'm going to leave
this comment as is for now. Best case, we decide merge should never expect
tombstones :)

+        // calling IncorporateRemoteDelete. While only MergeSyncData should be

[maxbogue, 2017/01/12 00:15:16]

s/. While/, while

[skym, 2017/01/12 17:44:00]

Done.

+        // passing in true for |include_local_only|. If this requirement
+        // changes, this DCHECK can change to act as a filter.
+        DCHECK(delete_from_local_.find(entry.key()) ==
+               delete_from_local_.end());
+        change_processor->Put(GetStorageKeyFromModel(entry.key()),
+                              CreateEntityData(entry),
+                              metadata_change_list.get());
+      }
+    }
+    return static_cast<AutofillMetadataChangeList*>(metadata_change_list.get())
+        ->TakeError();
+  }
+
+ private:
+  // There are three major outcomes of this method.
+  // 1. An error is encountered reading from the db, false is returned.
+  // 2. The entry is not found, |entry| will not be touched.
+  // 3. The entry is found, |entry| will be set.
+  bool ReadEntry(const AutofillKey& key, Optional<AutofillEntry>* entry) {
+    if (!InitializeIfNeeded()) {
+      return false;
+    }
+    std::set<AutofillEntry>::iterator iter =

[maxbogue, 2017/01/12 00:15:16]

auto

[skym, 2017/01/12 17:44:00]

Done.

+        unique_to_local_.find(AutofillEntry(key, Time(), Time()));
+    if (iter != unique_to_local_.end()) {
+      *entry = *iter;
+    }
+    return true;
+  }
+
+  bool InitializeIfNeeded() {
+    if (initialized_) {
+      return true;
+    }
+
+    std::vector<AutofillEntry> vector;
+    if (!table_->GetAllAutofillEntries(&vector)) {
+      return false;
+    }
+
+    unique_to_local_ = std::set<AutofillEntry>(vector.begin(), vector.end());
+    initialized_ = true;
+    return true;
+  }
+
+  AutofillTable* table_;
+
+  // This class attempts to lazily load data from |table_|. This field tracks
+  // if that has happened or not yet. To facilitate this, the first usage of
+  // |unique_to_local_| should typically be done through ReadEntry().

[maxbogue, 2017/01/12 00:15:16]

s/should typically/must

[skym, 2017/01/12 17:44:00]

I'm going to keep "should typically". The thing is, if what you want to
accomplish isn't reading a single entry (see FlushToSync()), then ReadEntry
doesn't work. It is awkward, but at least it's all contained within a fairly
small class.

+  bool initialized_ = false;
+
+  // Important to note that because AutofillEntry's operator > simply compares
+  // contained AutofillKeys, this acts as a map<AutofillKey, AutofillEntry>.
+  // This initially contains all local data, but as sync data is encountered,
+  // entries are removed from here.
+  std::set<AutofillEntry> unique_to_local_;
+
+  std::set<AutofillKey> delete_from_local_;
+  std::vector<AutofillEntry> save_to_local_;
+
+  // Contains current data for entries that are know to exist on both sync and

[maxbogue, 2017/01/12 00:15:16]

Maybe "Contains merged data for entries that existed on both sync and local
sides and need to be saved back to sync."?

[skym, 2017/01/12 17:44:00]

Done.

+  // local sides, as a result of merging timestamps, sync needs to be updated.
+  std::vector<AutofillEntry> save_to_sync_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncDifferenceTracker);
+};
+
 }  // namespace
 
 // static
@@ -71,8 +274,7 @@ void AutocompleteSyncBridge::CreateForWebDataServiceAndBackend(
   web_data_service->GetDBUserData()->SetUserData(
       UserDataKey(),
       new AutocompleteSyncBridge(
-          web_data_backend,
-          base::Bind(&syncer::ModelTypeChangeProcessor::Create)));
+          web_data_backend, base::Bind(&ModelTypeChangeProcessor::Create)));
 }
 
 // static
@@ -97,26 +299,54 @@ AutocompleteSyncBridge::~AutocompleteSyncBridge() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
-std::unique_ptr<syncer::MetadataChangeList>
+std::unique_ptr<MetadataChangeList>
 AutocompleteSyncBridge::CreateMetadataChangeList() {
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::MakeUnique<AutofillMetadataChangeList>(GetAutofillTable(),
                                                       syncer::AUTOFILL);
 }
 
-base::Optional<syncer::ModelError> AutocompleteSyncBridge::MergeSyncData(
-    std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
-    syncer::EntityDataMap entity_data_map) {
+Optional<syncer::ModelError> AutocompleteSyncBridge::MergeSyncData(
+    std::unique_ptr<MetadataChangeList> metadata_change_list,
+    EntityDataMap entity_data_map) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  NOTIMPLEMENTED();
+
+  // TODO(skym, crbug.com/680218): Uncomment and add unit tests.
+  /*SyncDifferenceTracker tracker(GetAutofillTable());
+  for (auto kv : entity_data_map) {
+    DCHECK(kv.second->specifics.has_autofill());
+    RETURN_IF(tracker.IncorporateRemoteSpecifics(
+        kv.first, kv.second->specifics.autofill()));
+  }
+
+  RETURN_IF(tracker.FlushToLocal(web_data_backend_));
+  RETURN_IF(tracker.FlushToSync(true, std::move(metadata_change_list),
+                                change_processor()));
+  web_data_backend_->RemoveExpiredFormElements();
+  web_data_backend_->NotifyThatSyncHasStarted(syncer::AUTOFILL);*/
   return {};
 }
 
-base::Optional<syncer::ModelError> AutocompleteSyncBridge::ApplySyncChanges(
-    std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
-    syncer::EntityChangeList entity_changes) {
+Optional<ModelError> AutocompleteSyncBridge::ApplySyncChanges(
+    std::unique_ptr<MetadataChangeList> metadata_change_list,
+    EntityChangeList entity_changes) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  NOTIMPLEMENTED();
+
+  SyncDifferenceTracker tracker(GetAutofillTable());
+  for (const EntityChange& change : entity_changes) {
+    if (change.type() == EntityChange::ACTION_DELETE) {
+      RETURN_IF(tracker.IncorporateRemoteDelete(change.storage_key()));
+    } else {
+      DCHECK(change.data().specifics.has_autofill());
+      RETURN_IF(tracker.IncorporateRemoteSpecifics(
+          change.storage_key(), change.data().specifics.autofill()));
+    }
+  }
+
+  RETURN_IF(tracker.FlushToLocal(web_data_backend_));
+  RETURN_IF(tracker.FlushToSync(false, std::move(metadata_change_list),
+                                change_processor()));
+  web_data_backend_->RemoveExpiredFormElements();
   return {};
 }
 
@@ -131,12 +361,9 @@ void AutocompleteSyncBridge::AutocompleteSyncBridge::GetData(
     return;
   }
 
-  std::unordered_set<std::string> keys_set;
-  for (const auto& key : storage_keys) {
-    keys_set.insert(key);
-  }
-
-  auto batch = base::MakeUnique<syncer::MutableDataBatch>();
+  std::unordered_set<std::string> keys_set(storage_keys.begin(),
+                                           storage_keys.end());
+  auto batch = base::MakeUnique<MutableDataBatch>();
   for (const AutofillEntry& entry : entries) {
     std::string key = GetStorageKeyFromModel(entry.key());
     if (keys_set.find(key) != keys_set.end()) {
@@ -156,7 +383,7 @@ void AutocompleteSyncBridge::GetAllData(DataCallback callback) {
     return;
   }
 
-  auto batch = base::MakeUnique<syncer::MutableDataBatch>();
+  auto batch = base::MakeUnique<MutableDataBatch>();
   for (const AutofillEntry& entry : entries) {
     batch->Put(GetStorageKeyFromModel(entry.key()), CreateEntityData(entry));
   }
@@ -164,9 +391,9 @@ void AutocompleteSyncBridge::GetAllData(DataCallback callback) {
 }
 
 std::string AutocompleteSyncBridge::GetClientTag(
-    const syncer::EntityData& entity_data) {
+    const EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
-  const sync_pb::AutofillSpecifics specifics = entity_data.specifics.autofill();
+  const AutofillSpecifics specifics = entity_data.specifics.autofill();
   return std::string(kAutocompleteEntryNamespaceTag) +
          net::EscapePath(specifics.name()) +
          std::string(kAutocompleteTagDelimiter) +
@@ -174,9 +401,9 @@ std::string AutocompleteSyncBridge::GetClientTag(
 }
 
 std::string AutocompleteSyncBridge::GetStorageKey(
-    const syncer::EntityData& entity_data) {
+    const EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
-  const sync_pb::AutofillSpecifics specifics = entity_data.specifics.autofill();
+  const AutofillSpecifics specifics = entity_data.specifics.autofill();
   return BuildSerializedStorageKey(specifics.name(), specifics.value());
 }
 
@@ -186,21 +413,6 @@ void AutocompleteSyncBridge::AutofillEntriesChanged(
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
-// static
-AutofillEntry AutocompleteSyncBridge::CreateAutofillEntry(
-    const sync_pb::AutofillSpecifics& autofill_specifics) {
-  AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()),
-                  base::UTF8ToUTF16(autofill_specifics.value()));
-  base::Time date_created, date_last_used;
-  const google::protobuf::RepeatedField<int64_t>& timestamps =
-      autofill_specifics.usage_timestamp();
-  if (!timestamps.empty()) {
-    date_created = base::Time::FromInternalValue(*timestamps.begin());
-    date_last_used = base::Time::FromInternalValue(*timestamps.rbegin());
-  }
-  return AutofillEntry(key, date_created, date_last_used);
-}
-
 AutofillTable* AutocompleteSyncBridge::GetAutofillTable() const {
   return AutofillTable::FromWebDatabase(web_data_backend_->GetDatabase());
 }