[Sync] ApplySyncChanges autofill implementation.

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

Index: components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
diff --git a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
index 4e7fd4232e3583ed5ffdbf0d409ff5b7be967d21..5ced7e42195bdbea2f43adad5b6c420c0eaa23bd 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
 
+#include <algorithm>
 #include <map>
 #include <vector>
 
@@ -21,13 +22,25 @@
 #include "components/sync/model/data_batch.h"
 #include "components/sync/model/fake_model_type_change_processor.h"
 #include "components/sync/model/metadata_batch.h"
+#include "components/sync/model/model_error.h"
 #include "components/webdata/common/web_database.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::ScopedTempDir;
+using base::Time;
 using sync_pb::AutofillSpecifics;
 using sync_pb::EntitySpecifics;
-using syncer::EntityDataPtr;
+using syncer::DataBatch;
 using syncer::EntityData;
+using syncer::EntityDataPtr;
+using syncer::EntityChange;
+using syncer::EntityChangeList;
+using syncer::FakeModelTypeChangeProcessor;
+using syncer::KeyAndData;
+using syncer::ModelError;
+using syncer::ModelType;
+using syncer::ModelTypeChangeProcessor;
+using syncer::ModelTypeSyncBridge;
 
 namespace autofill {
 
@@ -37,13 +50,25 @@ const char kNameFormat[] = "name %d";
 const char kValueFormat[] = "value %d";
 
 void VerifyEqual(const AutofillSpecifics& s1, const AutofillSpecifics& s2) {
-  EXPECT_EQ(s1.SerializeAsString(), s2.SerializeAsString());
+  // Instead of just comparing serialized strings, manually check fields to show
+  // differences on failure.
+  EXPECT_EQ(s1.name(), s2.name());
+  EXPECT_EQ(s1.value(), s2.value());
+  EXPECT_EQ(s1.usage_timestamp().size(), s2.usage_timestamp().size());
+  int size = std::min(s1.usage_timestamp().size(), s2.usage_timestamp().size());
+  for (int i = 0; i < size; ++i) {
+    EXPECT_EQ(s1.usage_timestamp(i), s2.usage_timestamp(i))
+        << "Values differ at index " << i;
+  }
+  // Neither should have any profile data, so we don't have to compare it.
+  EXPECT_FALSE(s1.has_profile());
+  EXPECT_FALSE(s2.has_profile());
 }
 
 void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected,
-                     std::unique_ptr<syncer::DataBatch> batch) {
+                     std::unique_ptr<DataBatch> batch) {
   while (batch->HasNext()) {
-    const syncer::KeyAndData& pair = batch->Next();
+    const KeyAndData& pair = batch->Next();
     auto iter = expected.find(pair.first);
     ASSERT_NE(iter, expected.end());
     VerifyEqual(iter->second, pair.second->specifics.autofill());
@@ -54,22 +79,22 @@ void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected,
   EXPECT_TRUE(expected.empty());
 }
 
-std::unique_ptr<syncer::ModelTypeChangeProcessor>
-CreateModelTypeChangeProcessor(syncer::ModelType type,
-                               syncer::ModelTypeSyncBridge* bridge) {
-  return base::MakeUnique<syncer::FakeModelTypeChangeProcessor>();
+std::unique_ptr<ModelTypeChangeProcessor> CreateModelTypeChangeProcessor(
+    ModelType type,
+    ModelTypeSyncBridge* bridge) {
+  return base::MakeUnique<FakeModelTypeChangeProcessor>();
 }
 
 AutofillEntry 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;
+  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());
+    date_created = Time::FromInternalValue(*timestamps.begin());
+    date_last_used = Time::FromInternalValue(*timestamps.rbegin());
   }
   return AutofillEntry(key, date_created, date_last_used);
 }
@@ -93,7 +118,7 @@ class FakeAutofillBackend : public AutofillWebDataBackend {
       autofill::AutofillWebDataServiceObserverOnDBThread* observer) override {}
   void RemoveExpiredFormElements() override {}
   void NotifyOfMultipleAutofillChanges() override {}
-  void NotifyThatSyncHasStarted(syncer::ModelType model_type) override {}
+  void NotifyThatSyncHasStarted(ModelType model_type) override {}
   void SetWebDatabase(WebDatabase* db) { db_ = db; }
 
  private:
@@ -128,14 +153,29 @@ class AutocompleteSyncBridgeTest : public testing::Test {
     table_.UpdateAutofillEntries(new_entries);
   }
 
-  AutofillSpecifics CreateSpecifics(int suffix) {
+  AutofillSpecifics CreateSpecifics(const std::string& name,
+                                    const std::string& value,
+                                    std::vector<int> timestamps) {
     AutofillSpecifics specifics;
-    specifics.set_name(base::StringPrintf(kNameFormat, suffix));
-    specifics.set_value(base::StringPrintf(kValueFormat, suffix));
-    specifics.add_usage_timestamp(0);
+    specifics.set_name(name);
+    specifics.set_value(value);
+    for (int timestamp : timestamps) {
+      specifics.add_usage_timestamp(
+          Time::FromTimeT(timestamp).ToInternalValue());
+    }
     return specifics;
   }
 
+  AutofillSpecifics CreateSpecifics(int suffix, std::vector<int> timestamps) {
+    return CreateSpecifics(base::StringPrintf(kNameFormat, suffix),
+                           base::StringPrintf(kValueFormat, suffix),
+                           timestamps);

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

std::move(timestamps)? Or make it a const& param in both functions

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

Made it const&. Done.

+  }
+
+  AutofillSpecifics CreateSpecifics(int suffix) {
+    return CreateSpecifics(suffix, std::vector<int>{0});
+  }
+
   std::string GetStorageKey(const AutofillSpecifics& specifics) {
     std::string key =
         bridge()->GetStorageKey(SpecificsToEntity(specifics).value());
@@ -143,8 +183,41 @@ class AutocompleteSyncBridgeTest : public testing::Test {
     return key;
   }
 
+  EntityChangeList EntityAddList(
+      const std::vector<AutofillSpecifics>& specifics_vector) {
+    EntityChangeList changes;
+    for (const auto& specifics : specifics_vector) {
+      changes.push_back(EntityChange::CreateAdd(GetStorageKey(specifics),
+                                                SpecificsToEntity(specifics)));
+    }
+    return changes;
+  }
+
+  void VerifyApplyChanges(const std::vector<EntityChange> changes) {

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

const ref instead of just const?

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

Done.

+    const base::Optional<ModelError> error = bridge()->ApplySyncChanges(

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

const auto error probs

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

Done.

+        bridge()->CreateMetadataChangeList(), changes);
+    EXPECT_FALSE(error);
+  }
+
+  void VerifyApplyAdds(const std::vector<AutofillSpecifics>& specifics) {
+    VerifyApplyChanges(EntityAddList(specifics));
+  }
+
+  std::map<std::string, AutofillSpecifics> ExpectedMap(
+      const std::vector<AutofillSpecifics>& specifics_vector) {
+    std::map<std::string, AutofillSpecifics> map;
+    for (const auto& specifics : specifics_vector) {
+      map[GetStorageKey(specifics)] = specifics;
+    }
+    return map;
+  }
+
+  void VerifyAllData(const std::vector<AutofillSpecifics>& expected) {
+    bridge()->GetAllData(base::Bind(&VerifyDataBatch, ExpectedMap(expected)));
+  }
+
  private:
-  base::ScopedTempDir temp_dir_;
+  ScopedTempDir temp_dir_;
   base::MessageLoop message_loop_;
   FakeAutofillBackend backend_;
   AutofillTable table_;
@@ -209,12 +282,9 @@ TEST_F(AutocompleteSyncBridgeTest, GetData) {
   const AutofillSpecifics specifics2 = CreateSpecifics(2);
   const AutofillSpecifics specifics3 = CreateSpecifics(3);
   SaveSpecificsToTable({specifics1, specifics2, specifics3});
-
-  const std::map<std::string, AutofillSpecifics> expected{
-      {GetStorageKey(specifics1), specifics1},
-      {GetStorageKey(specifics3), specifics3}};
-  bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics3)},
-                    base::Bind(&VerifyDataBatch, expected));
+  bridge()->GetData(
+      {GetStorageKey(specifics1), GetStorageKey(specifics3)},
+      base::Bind(&VerifyDataBatch, ExpectedMap({specifics1, specifics3})));
 }
 
 TEST_F(AutocompleteSyncBridgeTest, GetDataNotExist) {
@@ -222,13 +292,10 @@ TEST_F(AutocompleteSyncBridgeTest, GetDataNotExist) {
   const AutofillSpecifics specifics2 = CreateSpecifics(2);
   const AutofillSpecifics specifics3 = CreateSpecifics(3);
   SaveSpecificsToTable({specifics1, specifics2});
-
-  const std::map<std::string, AutofillSpecifics> expected{
-      {GetStorageKey(specifics1), specifics1},
-      {GetStorageKey(specifics2), specifics2}};
-  bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics2),
-                     GetStorageKey(specifics3)},
-                    base::Bind(&VerifyDataBatch, expected));
+  bridge()->GetData(
+      {GetStorageKey(specifics1), GetStorageKey(specifics2),
+       GetStorageKey(specifics3)},
+      base::Bind(&VerifyDataBatch, ExpectedMap({specifics1, specifics2})));
 }
 
 TEST_F(AutocompleteSyncBridgeTest, GetAllData) {
@@ -236,12 +303,126 @@ TEST_F(AutocompleteSyncBridgeTest, GetAllData) {
   const AutofillSpecifics specifics2 = CreateSpecifics(2);
   const AutofillSpecifics specifics3 = CreateSpecifics(3);
   SaveSpecificsToTable({specifics1, specifics2, specifics3});
+  VerifyAllData({specifics1, specifics2, specifics3});
+}
+
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesEmpty) {
+  // TODO(skym, crbug.com/672619): Ideally would like to verify that the db is
+  // not accessed.
+  VerifyApplyAdds(std::vector<AutofillSpecifics>());
+}
+
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesSimple) {
+  AutofillSpecifics specifics1 = CreateSpecifics(1);
+  AutofillSpecifics specifics2 = CreateSpecifics(2);
+  ASSERT_NE(specifics1.SerializeAsString(), specifics2.SerializeAsString());
+  ASSERT_NE(GetStorageKey(specifics1), GetStorageKey(specifics2));
+
+  VerifyApplyAdds({specifics1, specifics2});
+  VerifyAllData({specifics1, specifics2});
+
+  VerifyApplyChanges({EntityChange::CreateDelete(GetStorageKey(specifics1))});
+  VerifyAllData({specifics2});
+}
+
+// Should be resilient to deleting and updating non-existent things, and adding
+// existing ones.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesWrongChangeType) {
+  AutofillSpecifics specifics = CreateSpecifics(1, {1});
+  VerifyApplyChanges({EntityChange::CreateDelete(GetStorageKey(specifics))});
+  VerifyAllData(std::vector<AutofillSpecifics>());
+
+  VerifyApplyChanges({EntityChange::CreateUpdate(
+      GetStorageKey(specifics), SpecificsToEntity(specifics))});
+  VerifyAllData({specifics});
+
+  specifics.add_usage_timestamp(Time::FromTimeT(2).ToInternalValue());
+  VerifyApplyAdds({specifics});
+  VerifyAllData({specifics});
+}
+
+// The format in the table has a fixed 2 timestamp format. Round tripping is
+// lossy and the middle value should be thrown out.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesThreeTimestamps) {
+  VerifyApplyAdds({CreateSpecifics(1, {1, 2, 3})});
+  VerifyAllData({CreateSpecifics(1, {1, 3})});
+}
+
+// The correct format of timestamps is that the first should be smaller and the
+// second should be larger. Bad foreign data should be repaired.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesWrongOrder) {
+  VerifyApplyAdds({CreateSpecifics(1, {3, 2})});
+  VerifyAllData({CreateSpecifics(1, {2, 3})});
+}
+
+// In a minor attempt to save bandwidth, we only send one of the two timestamps
+// when they share a value.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesRepeatedTime) {
+  VerifyApplyAdds({CreateSpecifics(1, {2, 2})});
+  VerifyAllData({CreateSpecifics(1, {2})});
+}
+
+// Again, the format in the table is lossy, and cannot distinguish between no
+// time, and valid time zero.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesNoTime) {
+  VerifyApplyAdds({CreateSpecifics(1, std::vector<int>())});
+  VerifyAllData({CreateSpecifics(1, {0})});
+}
+
+// If has_value() returns false, then the specifics are determined to be old
+// style and ignored.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesNoValue) {
+  AutofillSpecifics input = CreateSpecifics(1, {2, 3});
+  input.clear_value();
+  VerifyApplyAdds({input});
+  VerifyAllData(std::vector<AutofillSpecifics>());
+}
+
+// Should be treated the same as an empty string name. This inconsistency is
+// being perpetuated from the previous sync integration.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesNoName) {
+  AutofillSpecifics input = CreateSpecifics(1, {2, 3});
+  input.clear_name();
+  VerifyApplyAdds({input});
+  VerifyAllData({input});
+}
+
+// UTF-8 characters should not be dropped when round tripping, including middle
+// of string \0 characters.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesUTF) {
+  const AutofillSpecifics specifics =
+      CreateSpecifics(std::string("\n\0\x12\0", 4), "\xEC\xA4\x91", {1});
+  VerifyApplyAdds({specifics});
+  VerifyAllData({specifics});
+}
+
+// Timestamps should always use the oldest creation time, and the most recent
+// usage time.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesMinMaxTimestamps) {
+  const AutofillSpecifics initial = CreateSpecifics(1, {3, 6});
+  VerifyApplyAdds({initial});
+  VerifyAllData({initial});
+
+  VerifyApplyAdds({CreateSpecifics(1, {2, 5})});
+  VerifyAllData({CreateSpecifics(1, {2, 6})});
+
+  VerifyApplyAdds({CreateSpecifics(1, {4, 7})});
+  VerifyAllData({CreateSpecifics(1, {2, 7})});
+}
+
+// An error should be generated when parsing the storage key happens. This
+// should never happen in practice because storage keys should be generated at
+// runtime by the bridge and not loaded from disk.
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesBadStorageKey) {
+  const base::Optional<ModelError> error = bridge()->ApplySyncChanges(

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

const auto error?

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

Done.

+      bridge()->CreateMetadataChangeList(),
+      {EntityChange::CreateDelete("bogus storage key")});
+  EXPECT_TRUE(error);
+}
 
-  const std::map<std::string, AutofillSpecifics> expected{
-      {GetStorageKey(specifics1), specifics1},
-      {GetStorageKey(specifics2), specifics2},
-      {GetStorageKey(specifics3), specifics3}};
-  bridge()->GetAllData(base::Bind(&VerifyDataBatch, expected));
+TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesDatabaseFailure) {
+  // TODO(skym, crbug.com/672619): Should have tests that get false back when
+  // making db calls and verify the errors are propagated up.
 }
 
 }  // namespace autofill