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

Previously we were going to use a | delimiter and manually construct
storage keys ourselves. The requires us to write logic to reverse it,
and is inefficient when encoding characters that need to be escaped.
The disadvantages are that we always have 4 bytes (instead of 1) of
overhead, and printing the string is less readable.

BUG=675992

Review-Url: https://codereview.chromium.org/2598113002
Cr-Commit-Position: refs/heads/master@{#442011}

components/autofill/core/browser/proto/BUILD.gn

@@ -6,6 +6,7 @@ import("//third_party/protobuf/proto_library.gni")
 
 proto_library("proto") {
   sources = [
+    "autofill_sync.proto",
     "server.proto",
   ]
 }

components/autofill/core/browser/proto/autofill_sync.proto

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+package autofill;
+
+// Used to convert between autofill::AutofillKey and a std::string that can be
+// passed to sync as storage key to uniquely identify an entity of ModelType
+// syncer::AUTOFILL.
+message AutofillSyncStorageKey {
+  optional string name = 1;
+  optional string value = 2;
+}

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

@@ -5,10 +5,13 @@
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
 
 #include <unordered_set>
+#include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/autofill/core/browser/proto/autofill_sync.pb.h"
 #include "components/autofill/core/browser/webdata/autofill_metadata_change_list.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_backend.h"
@@ -19,9 +22,12 @@
 #include "components/sync/model/sync_error.h"
 #include "net/base/escape.h"
 
+namespace autofill {
+
 namespace {
 
 const char kAutocompleteEntryNamespaceTag[] = "autofill_entry|";
+const char kAutocompleteTagDelimiter[] = "|";
 
 void* UserDataKey() {
   // Use the address of a static that COMDAT folding won't ever collide
@@ -31,7 +37,7 @@ void* UserDataKey() {
 }
 
 std::unique_ptr<syncer::EntityData> CreateEntityData(
-    const autofill::AutofillEntry& entry) {
+    const AutofillEntry& entry) {
   auto entity_data = base::MakeUnique<syncer::EntityData>();
   entity_data->non_unique_name = base::UTF16ToUTF8(entry.key().name());
   sync_pb::AutofillSpecifics* autofill =
@@ -44,9 +50,20 @@ std::unique_ptr<syncer::EntityData> CreateEntityData(
   return entity_data;
 }
 
-}  // namespace
+std::string BuildSerializedStorageKey(const std::string& name,
+                                      const std::string& value) {
+  AutofillSyncStorageKey proto;
+  proto.set_name(name);
+  proto.set_value(value);
+  return proto.SerializeAsString();
+}
 
-namespace autofill {
+std::string GetStorageKeyFromModel(const AutofillKey& key) {
+  return BuildSerializedStorageKey(base::UTF16ToUTF8(key.name()),
+                                   base::UTF16ToUTF8(key.value()));
+}
+
+}  // namespace
 
 // static
 void AutocompleteSyncBridge::CreateForWebDataServiceAndBackend(
@@ -117,7 +134,7 @@ void AutocompleteSyncBridge::AutocompleteSyncBridge::GetData(
   std::vector<AutofillEntry> entries;
   GetAutofillTable()->GetAllAutofillEntries(&entries);
   for (const AutofillEntry& entry : entries) {
-    std::string key = GetStorageKeyFromAutofillEntry(entry);
+    std::string key = GetStorageKeyFromModel(entry.key());
     if (keys_set.find(key) != keys_set.end()) {
       batch->Put(key, CreateEntityData(entry));
     }
@@ -131,7 +148,7 @@ void AutocompleteSyncBridge::GetAllData(DataCallback callback) {
   std::vector<AutofillEntry> entries;
   GetAutofillTable()->GetAllAutofillEntries(&entries);
   for (const AutofillEntry& entry : entries) {
-    batch->Put(GetStorageKeyFromAutofillEntry(entry), CreateEntityData(entry));
+    batch->Put(GetStorageKeyFromModel(entry.key()), CreateEntityData(entry));
   }
   callback.Run(syncer::SyncError(), std::move(batch));
 }
@@ -139,18 +156,18 @@ void AutocompleteSyncBridge::GetAllData(DataCallback callback) {
 std::string AutocompleteSyncBridge::GetClientTag(
     const syncer::EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_autofill());
-
   const sync_pb::AutofillSpecifics specifics = entity_data.specifics.autofill();
-  std::string storage_key =
-      FormatStorageKey(specifics.name(), specifics.value());
-  std::string prefix(kAutocompleteEntryNamespaceTag);
-  return prefix + storage_key;
+  return std::string(kAutocompleteEntryNamespaceTag) +
+         net::EscapePath(specifics.name()) +
+         std::string(kAutocompleteTagDelimiter) +
+         net::EscapePath(specifics.value());
 }
 
 std::string AutocompleteSyncBridge::GetStorageKey(
     const syncer::EntityData& entity_data) {
+  DCHECK(entity_data.specifics.has_autofill());
   const sync_pb::AutofillSpecifics specifics = entity_data.specifics.autofill();
-  return FormatStorageKey(specifics.name(), specifics.value());
+  return BuildSerializedStorageKey(specifics.name(), specifics.value());
 }
 
 // AutofillWebDataServiceObserverOnDBThread implementation.
@@ -178,16 +195,4 @@ AutofillTable* AutocompleteSyncBridge::GetAutofillTable() const {
   return AutofillTable::FromWebDatabase(web_data_backend_->GetDatabase());
 }
 
-std::string AutocompleteSyncBridge::GetStorageKeyFromAutofillEntry(
-    const autofill::AutofillEntry& entry) {
-  return FormatStorageKey(base::UTF16ToUTF8(entry.key().name()),
-                          base::UTF16ToUTF8(entry.key().value()));
-}
-
-// static
-std::string AutocompleteSyncBridge::FormatStorageKey(const std::string& name,
-                                                     const std::string& value) {
-  return net::EscapePath(name) + "|" + net::EscapePath(value);
-}
-
 }  // namespace autofill

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

@@ -5,6 +5,9 @@
 #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOCOMPLETE_SYNC_BRIDGE_H_
 #define COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOCOMPLETE_SYNC_BRIDGE_H_
 
+#include <memory>
+#include <string>
+
 #include "base/scoped_observer.h"
 #include "base/supports_user_data.h"
 #include "base/threading/non_thread_safe.h"

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,10 @@ class AutocompleteSyncBridgeTest : public testing::Test {
   }
 
   std::string GetStorageKey(const AutofillSpecifics& specifics) {
-    return net::EscapePath(specifics.name()) + "|" +
-           net::EscapePath(specifics.value());
+    std::string key =
+        bridge()->GetStorageKey(SpecificsToEntity(specifics).value());
+    EXPECT_FALSE(key.empty());
+    return key;
   }
 
  private:
@@ -131,6 +157,56 @@ class AutocompleteSyncBridgeTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest);
 };
 
+TEST_F(AutocompleteSyncBridgeTest, GetClientTag) {
+  // TODO(skym, crbug.com/675991): Implementation.
+}
+
+TEST_F(AutocompleteSyncBridgeTest, GetStorageKey) {
+  std::string key = GetStorageKey(CreateSpecifics(1));
+  EXPECT_EQ(key, GetStorageKey(CreateSpecifics(1)));
+  EXPECT_NE(key, GetStorageKey(CreateSpecifics(2)));
+}
+
+// Timestamps should not affect storage keys.
+TEST_F(AutocompleteSyncBridgeTest, GetStorageKeyTimestamp) {
+  AutofillSpecifics specifics = CreateSpecifics(1);
+  std::string key = GetStorageKey(specifics);
+
+  specifics.add_usage_timestamp(1);
+  EXPECT_EQ(key, GetStorageKey(specifics));
+
+  specifics.add_usage_timestamp(0);
+  EXPECT_EQ(key, GetStorageKey(specifics));
+
+  specifics.add_usage_timestamp(-1);
+  EXPECT_EQ(key, GetStorageKey(specifics));
+}
+
+// Verify that the \0 character is respected as a difference.
+TEST_F(AutocompleteSyncBridgeTest, GetStorageKeyNull) {
+  AutofillSpecifics specifics;
+  std::string key = GetStorageKey(specifics);
+
+  specifics.set_value(std::string("\0", 1));
+  EXPECT_NE(key, GetStorageKey(specifics));
+}
+
+// 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);