[Sync] Use a proto to generate AutofillSyncStorageKey's storage keys.

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 de05b01f299f68c6e6ee03e167e1d7d42c521c44..ca5e4ef35df1597714867a23025dd403fe8cae77 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
@@ -4,7 +4,8 @@
 
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
 
-#include <memory>
+#include <map>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/files/scoped_temp_dir.h"
@@ -21,10 +22,12 @@
 #include "components/sync/model/fake_model_type_change_processor.h"
 #include "components/sync/model/metadata_batch.h"
 #include "components/webdata/common/web_database.h"
-#include "net/base/escape.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using sync_pb::AutofillSpecifics;
+using sync_pb::EntitySpecifics;
+using syncer::EntityDataPtr;
+using syncer::EntityData;
 using syncer::SyncError;
 
 namespace autofill {
@@ -60,6 +63,28 @@ CreateModelTypeChangeProcessor(syncer::ModelType type,
   return base::MakeUnique<syncer::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;
+  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);
+}
+
+// Creates an EntityData/EntityDataPtr around a copy of the given specifics.
+EntityDataPtr SpecificsToEntity(const AutofillSpecifics& specifics) {
+  EntityData data;
+  data.client_tag_hash = "ignored";
+  *data.specifics.mutable_autofill() = specifics;
+  return data.PassToPtr();
+}
+
 class FakeAutofillBackend : public AutofillWebDataBackend {
  public:
   FakeAutofillBackend() {}
@@ -101,8 +126,7 @@ class AutocompleteSyncBridgeTest : public testing::Test {
       const std::vector<AutofillSpecifics>& specifics_list) {
     std::vector<AutofillEntry> new_entries;
     for (const auto& specifics : specifics_list) {
-      new_entries.push_back(
-          AutocompleteSyncBridge::CreateAutofillEntry(specifics));
+      new_entries.push_back(CreateAutofillEntry(specifics));
     }
     table_.UpdateAutofillEntries(new_entries);
   }
@@ -116,8 +140,7 @@ class AutocompleteSyncBridgeTest : public testing::Test {
   }
 
   std::string GetStorageKey(const AutofillSpecifics& specifics) {
-    return net::EscapePath(specifics.name()) + "|" +
-           net::EscapePath(specifics.value());
+    return bridge()->GetStorageKey(SpecificsToEntity(specifics).value());
   }
 
  private:
@@ -131,6 +154,63 @@ class AutocompleteSyncBridgeTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest);
 };
 
+TEST_F(AutocompleteSyncBridgeTest, GetClientTag) {
+  // TODO(skym, crbug.com/675991): Implementation.
+
+  AutofillSpecifics specifics2 = CreateSpecifics(2);
+  specifics2.set_name("a|b|z");
+  specifics2.set_value("a|b|c\xEC\xA4\x91");
+  LOG(ERROR) << "SKYM "
+             << bridge()->GetClientTag(SpecificsToEntity(specifics2).value());
+}
+
+TEST_F(AutocompleteSyncBridgeTest, GetStorageKeyRelative) {
+  AutofillSpecifics specifics1 = CreateSpecifics(1);
+  AutofillSpecifics specifics2 = CreateSpecifics(2);
+  std::string key_1_no_time = GetStorageKey(specifics1);
+  std::string key_2_no_time = GetStorageKey(specifics2);
+
+  specifics1.add_usage_timestamp(123);
+  specifics1.add_usage_timestamp(1234);
+  specifics1.add_usage_timestamp(12345);
+  specifics2.add_usage_timestamp(-12345);
+  std::string key_1_with_time = GetStorageKey(specifics1);
+  std::string key_2_with_time = GetStorageKey(specifics2);
+
+  std::string key_empty =
+      bridge()->GetStorageKey(SpecificsToEntity(AutofillSpecifics()).value());

[maxbogue, 2017/01/04 01:42:46]

I don't really think key_empty is adding anything to this test case. I'd just
make it test that the keys don't change with timestamps.

[skym, 2017/01/05 01:31:51]

Mostly removed. Added a new case comparing empty and \0.

+
+  EXPECT_FALSE(key_1_no_time.empty());

[maxbogue, 2017/01/04 01:42:46]

Consider replacing all these with a check inside GetStorageKey().

[skym, 2017/01/05 01:31:51]

Done.

+  EXPECT_FALSE(key_1_with_time.empty());
+  EXPECT_FALSE(key_2_no_time.empty());
+  EXPECT_FALSE(key_2_with_time.empty());
+  EXPECT_FALSE(key_empty.empty());
+
+  // Timestamps should not affect storage keys.
+  EXPECT_EQ(key_1_no_time, key_1_with_time);
+  EXPECT_EQ(key_2_no_time, key_2_with_time);
+
+  EXPECT_NE(key_1_no_time, key_2_no_time);

[maxbogue, 2017/01/04 01:42:46]

These are pretty redundant with the hard-coded assertions in GetStorageKeyFixed,
no?

[skym, 2017/01/05 01:31:51]

Removed most of these, and moved some stuff around.

+  EXPECT_NE(key_1_no_time, key_empty);
+  EXPECT_NE(key_2_no_time, key_empty);
+}
+
+// The storage key should never accidentally change for existing data. This
+// would cause lookups to fail and either lose or duplicate user data. It should
+// be possible for the model type to migrate storage key formats, but doing so
+// would need to be done very carefully.
+TEST_F(AutocompleteSyncBridgeTest, GetStorageKeyFixed) {
+  EXPECT_EQ("\n\x6name 1\x12\avalue 1", GetStorageKey(CreateSpecifics(1)));
+  EXPECT_EQ("\n\x6name 2\x12\avalue 2", GetStorageKey(CreateSpecifics(2)));
+  // This literal contains the null terminating character, which causes
+  // std::string to stop copying early if we don't tell it how much to read.
+  EXPECT_EQ(std::string("\n\0\x12\0", 4), GetStorageKey(AutofillSpecifics()));
+  AutofillSpecifics specifics;
+  specifics.set_name("\xEC\xA4\x91");
+  specifics.set_value("\xD0\x80");
+  EXPECT_EQ("\n\x3\xEC\xA4\x91\x12\x2\xD0\x80", GetStorageKey(specifics));
+}
+
 TEST_F(AutocompleteSyncBridgeTest, GetData) {
   const AutofillSpecifics specifics1 = CreateSpecifics(1);
   const AutofillSpecifics specifics2 = CreateSpecifics(2);