[sync] Handle local changes in AutocompleteSyncBridge

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..b4a519c33633e59895cbc4b1211aedbf1875077d 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
@@ -14,16 +14,23 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
+#include "components/autofill/core/browser/webdata/autofill_entry.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_backend.h"
-#include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
+#include "components/autofill/core/browser/webdata/autofill_webdata_backend_impl.h"
 #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/recording_model_type_change_processor.h"
 #include "components/webdata/common/web_database.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::ASCIIToUTF16;
+using base::Time;
+using base::TimeDelta;
 using sync_pb::AutofillSpecifics;
 using sync_pb::EntitySpecifics;
 using syncer::EntityDataPtr;
@@ -54,13 +61,7 @@ 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>();
-}
-
-AutofillEntry CreateAutofillEntry(
+AutofillEntry CreateAutofillEntryFromSpecifics(
     const sync_pb::AutofillSpecifics& autofill_specifics) {
   AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()),
                   base::UTF8ToUTF16(autofill_specifics.value()));
@@ -74,7 +75,29 @@ AutofillEntry CreateAutofillEntry(
   return AutofillEntry(key, date_created, date_last_used);
 }
 
-// Creates an EntityData/EntityDataPtr around a copy of the given specifics.
+AutofillEntry CreateAutofillEntryFromValues(const char* name,
+                                            const char* value,
+                                            int time_shift0,
+                                            int time_shift1) {
+  // Time deep in the past would cause Autocomplete sync to discard the
+  // entries.
+  static Time base_time = Time::Now().LocalMidnight();
+
+  Time date_created = base_time + TimeDelta::FromSeconds(time_shift0);
+  Time date_last_used = date_created;
+  if (time_shift1 >= 0) {
+    date_last_used = base_time + TimeDelta::FromSeconds(time_shift1);
+  }
+  return AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)),
+                       date_created, date_last_used);
+}
+
+AutofillEntry CreateAutofillEntryFromValues(const char* name,
+                                            const char* value,
+                                            int time_shift) {
+  return CreateAutofillEntryFromValues(name, value, time_shift, -1);

[skym, 2017/01/12 00:18:32]

I really dislike this -1 approach.

[skym, 2017/01/12 00:40:29]

Like, what do you think of maybe just passing time_shift as 3rd and 4th params
and delete lines 87,88,90?

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+}
+
 EntityDataPtr SpecificsToEntity(const AutofillSpecifics& specifics) {
   EntityData data;
   data.client_tag_hash = "ignored";
@@ -82,6 +105,8 @@ EntityDataPtr SpecificsToEntity(const AutofillSpecifics& specifics) {
   return data.PassToPtr();
 }
 
+}  // namespace

[maxbogue, 2017/01/12 01:33:36]

Why'd you take the FakeAutofillBackend out of the anonymous namespace?

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+
 class FakeAutofillBackend : public AutofillWebDataBackend {
  public:
   FakeAutofillBackend() {}
@@ -100,30 +125,31 @@ class FakeAutofillBackend : public AutofillWebDataBackend {
   WebDatabase* db_;
 };
 
-}  // namespace
-
 class AutocompleteSyncBridgeTest : public testing::Test {
- public:
-  AutocompleteSyncBridgeTest() {
-    if (temp_dir_.CreateUniqueTempDir()) {
-      db_.AddTable(&table_);
-      db_.Init(temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"));
-      backend_.SetWebDatabase(&db_);
-
-      bridge_.reset(new AutocompleteSyncBridge(
-          &backend_, base::Bind(&CreateModelTypeChangeProcessor)));
-    }
-  }
-  ~AutocompleteSyncBridgeTest() override {}
-
  protected:

[maxbogue, 2017/01/12 01:33:36]

I don't understand the protected thing in tests. Just have public and private
IMO.

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+  // A non-owning pointer to the processor given to the bridge. Will be null
+  // before being given to the bridge, to make ownership easier.
+  syncer::RecordingModelTypeChangeProcessor* processor_ = nullptr;

[maxbogue, 2017/01/12 01:33:36]

This variable has an accessor, it should just be private.

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+  AutofillTable table_;

[maxbogue, 2017/01/12 01:33:36]

Give this variable an accessor and make it private.

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+
   AutocompleteSyncBridge* bridge() { return bridge_.get(); }
 
+  syncer::RecordingModelTypeChangeProcessor* processor() { return processor_; }
+
+  std::unique_ptr<syncer::ModelTypeChangeProcessor>
+  CreateModelTypeChangeProcessor(syncer::ModelType type,
+                                 syncer::ModelTypeSyncBridge* bridge) {
+    auto processor =
+        base::MakeUnique<syncer::RecordingModelTypeChangeProcessor>();
+    processor_ = processor.get();
+    return std::move(processor);
+  }
+
   void SaveSpecificsToTable(
       const std::vector<AutofillSpecifics>& specifics_list) {
     std::vector<AutofillEntry> new_entries;
     for (const auto& specifics : specifics_list) {
-      new_entries.push_back(CreateAutofillEntry(specifics));
+      new_entries.push_back(CreateAutofillEntryFromSpecifics(specifics));
     }
     table_.UpdateAutofillEntries(new_entries);
   }
@@ -143,15 +169,34 @@ class AutocompleteSyncBridgeTest : public testing::Test {
     return key;
   }
 
+  std::string GetStorageKeyFromModel(const AutofillKey& key) {
+    return bridge()->GetStorageKeyFromModel(key);
+  }
+
  private:
   base::ScopedTempDir temp_dir_;
   base::MessageLoop message_loop_;
   FakeAutofillBackend backend_;
-  AutofillTable table_;
   WebDatabase db_;
   std::unique_ptr<AutocompleteSyncBridge> bridge_;
 
   DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest);
+
+ public:

[maxbogue, 2017/01/12 01:33:36]

plz put public section above the protected one for my sanity. Actually since I'm
also asking you to get rid of the protected section please put these things at
the top of the new public section.

[Patrick Noland, 2017/01/13 23:31:52]

Done.

+  AutocompleteSyncBridgeTest() {
+    if (temp_dir_.CreateUniqueTempDir()) {
+      db_.AddTable(&table_);
+      db_.Init(temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"));
+      backend_.SetWebDatabase(&db_);
+
+      bridge_.reset(new AutocompleteSyncBridge(
+          &backend_,
+          base::Bind(
+              &AutocompleteSyncBridgeTest::CreateModelTypeChangeProcessor,
+              base::Unretained(this))));
+    }
+  }
+  ~AutocompleteSyncBridgeTest() override {}
 };
 
 TEST_F(AutocompleteSyncBridgeTest, GetClientTag) {
@@ -244,4 +289,122 @@ TEST_F(AutocompleteSyncBridgeTest, GetAllData) {
   bridge()->GetAllData(base::Bind(&VerifyDataBatch, expected));
 }
 
-}  // namespace autofill
+TEST_F(AutocompleteSyncBridgeTest, LocalEntriesAdded) {
+  const AutofillEntry added_entry1(
+      CreateAutofillEntryFromValues("added", "entry1", 1));
+  const AutofillEntry added_entry2(
+      CreateAutofillEntryFromValues("added", "entry2", 1));
+
+  table_.UpdateAutofillEntries({added_entry1, added_entry2});
+
+  const AutofillKey entry_key1 = added_entry1.key();
+  const AutofillKey entry_key2 = added_entry2.key();
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::ADD, entry_key1),
+       AutofillChange(AutofillChange::ADD, entry_key2)});
+
+  EXPECT_EQ(2u, processor()->put_multimap().size());
+
+  const auto& recorded_specifics_for_key1 =
+      processor()->put_multimap().find(GetStorageKeyFromModel(entry_key1));

[skym, 2017/01/12 00:18:32]

What do you think of creating the model objects, making the changes, and then
recreating the same expected objects manually here as specifics and verify we
see those? Instead of calling this private method.

[Patrick Noland, 2017/01/13 23:31:52]

Done. I've added a method that converts from an AutofillEntry to specifics to
make this less tedious

+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key1);
+  sync_pb::AutofillSpecifics specifics =
+      recorded_specifics_for_key1->second->specifics.autofill();
+  EXPECT_EQ(specifics.value(), "entry1");
+
+  const auto& recorded_specifics_for_key2 =
+      processor()->put_multimap().find(GetStorageKeyFromModel(entry_key2));
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key2);
+  specifics = recorded_specifics_for_key2->second->specifics.autofill();
+  EXPECT_EQ(specifics.value(), "entry2");
+}
+
+TEST_F(AutocompleteSyncBridgeTest, LocalEntryAddedThenUpdated) {
+  AutofillEntry added_entry(CreateAutofillEntryFromValues("my", "entry", 1));
+  table_.UpdateAutofillEntries({added_entry});
+  AutofillKey entry_key = added_entry.key();
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::ADD, entry_key)});
+
+  EXPECT_EQ(1u, processor()->put_multimap().size());
+
+  const std::string storage_key = GetStorageKeyFromModel(entry_key);
+  auto recorded_specifics_for_key =
+      processor()->put_multimap().find(storage_key);
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key);
+
+  sync_pb::AutofillSpecifics specifics =
+      recorded_specifics_for_key->second->specifics.autofill();
+  EXPECT_EQ(added_entry.date_last_used(),
+            Time::FromInternalValue(specifics.usage_timestamp(0)));
+
+  AutofillEntry updated_entry(
+      CreateAutofillEntryFromValues("my", "entry", 1, 4));
+  table_.UpdateAutofillEntries({updated_entry});
+  AutofillKey updated_key = updated_entry.key();
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::UPDATE, entry_key)});
+
+  // RecordingModelTypeChangeProcessor's put_multimap records each put for a
+  // given key in an ordered list, so we need to get the iterator for this list
+  // and then advance it to find the update we just applied.
+  recorded_specifics_for_key = processor()->put_multimap().find(storage_key);
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key);
+  recorded_specifics_for_key++;
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key);
+
+  specifics = recorded_specifics_for_key->second->specifics.autofill();
+  EXPECT_EQ(2, specifics.usage_timestamp_size());
+  EXPECT_EQ(updated_entry.date_last_used(),
+            Time::FromInternalValue(specifics.usage_timestamp(1)));
+}
+
+TEST_F(AutocompleteSyncBridgeTest, LocalEntryDeleted) {
+  AutofillEntry deleted_entry(CreateAutofillEntryFromValues("my", "entry", 1));
+
+  AutofillKey deleted_key = deleted_entry.key();
+  const std::string storage_key = GetStorageKeyFromModel(deleted_key);
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::REMOVE, deleted_key)});
+
+  EXPECT_EQ(1u, processor()->delete_set().size());
+  const auto& deleted_entries = processor()->delete_set().find(storage_key);
+  EXPECT_NE(deleted_entries, processor()->delete_set().end());
+}
+
+// Helper struct to track callback invocation status.
+struct TestForRun {

[skym, 2017/01/12 00:18:32]

Should this get moved to up above the test cases?

[maxbogue, 2017/01/12 01:33:36]

(yes, inside the anonymous namespace)

[Patrick Noland, 2017/01/13 23:31:52]

n/a now that flare is gone

+  TestForRun() : cb(base::Bind(&TestForRun::Run, base::Unretained(this))) {}
+
+  void Run(syncer::ModelType type) { cb_run = true; }
+
+  bool cb_run;
+  AutocompleteSyncBridge::StartSyncFlare cb;
+};
+
+TEST_F(AutocompleteSyncBridgeTest, TestSyncFlare) {
+  processor()->SetIsTrackingMetadata(false);
+
+  TestForRun tfr;
+  AutocompleteSyncBridge::StartSyncFlare flare = tfr.cb;
+
+  bridge()->InjectStartSyncFlare(flare);
+
+  AutofillEntry deleted_entry(CreateAutofillEntryFromValues("my", "entry", 1));
+
+  AutofillKey deleted_key = deleted_entry.key();
+  const std::string storage_key = GetStorageKeyFromModel(deleted_key);
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::REMOVE, deleted_key)});
+
+  // The processor shouldn't be called if it's not yet tracking metadata.
+  EXPECT_EQ(0u, processor()->delete_set().size());
+  EXPECT_TRUE(tfr.cb_run);
+}
+
+};  // namespace autofill

[maxbogue, 2017/01/12 01:33:36]

No semi-colon, that's just for classes because C++ hates consistency.

[Patrick Noland, 2017/01/13 23:31:52]

Done.