[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 515b874815c6918f967a21bb325e5954b0a078c2..0ce537b1966ee7b6f85cc47fadbf2571113830b6 100644
--- a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
@@ -15,19 +15,25 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"

[skym, 2017/01/14 00:04:34]

Remove this, duplicate with line 17.

[Patrick Noland, 2017/01/17 22:59:55]

Done.

 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"

[skym, 2017/01/14 00:04:34]

Remove this, duplicate with line 5.

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+#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/model_error.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::ScopedTempDir;
 using base::Time;
+using base::TimeDelta;
 using sync_pb::AutofillSpecifics;
 using sync_pb::EntitySpecifics;
 using syncer::DataBatch;
@@ -79,14 +85,8 @@ void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected,
   EXPECT_TRUE(expected.empty());
 }
 
-std::unique_ptr<ModelTypeChangeProcessor> CreateModelTypeChangeProcessor(
-    ModelType type,
-    ModelTypeSyncBridge* bridge) {
-  return base::MakeUnique<FakeModelTypeChangeProcessor>();
-}
-
-AutofillEntry CreateAutofillEntry(
-    const sync_pb::AutofillSpecifics& autofill_specifics) {
+AutofillEntry CreateAutofillEntryFromSpecifics(
+    const AutofillSpecifics& autofill_specifics) {
   AutofillKey key(base::UTF8ToUTF16(autofill_specifics.name()),
                   base::UTF8ToUTF16(autofill_specifics.value()));
   Time date_created, date_last_used;
@@ -99,7 +99,32 @@ AutofillEntry CreateAutofillEntry(
   return AutofillEntry(key, date_created, date_last_used);
 }
 
-// Creates an EntityData/EntityDataPtr around a copy of the given specifics.
+AutofillSpecifics CreateAutofillSpecificsFromEntry(const AutofillEntry& entry) {

[skym, 2017/01/14 00:04:34]

Part of me wonders if maybe it'd be the most clean if all the tests start off
creating either model or specifics objects. And then convert in a single
direction? I feel like the tests are a bit inconsistent, WDYT?

[Patrick Noland, 2017/01/17 22:59:55]

This adds a couple of lines to each test but is more consistent and makes the
helper code cleaner so overall it's a net win. Done.

+  AutofillSpecifics 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());
+  if (entry.date_created() != entry.date_last_used()) {
+    autofill.add_usage_timestamp(entry.date_last_used().ToInternalValue());
+  }
+
+  return autofill;
+}
+
+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

[skym, 2017/01/14 00:04:34]

Technically not true right now. See line 144, we don't do anything.

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+  // entries.
+  static Time base_time = Time::Now().LocalMidnight();
+
+  Time date_created = base_time + TimeDelta::FromSeconds(time_shift0);
+  Time date_last_used = base_time + TimeDelta::FromSeconds(time_shift1);
+  return AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)),

[skym, 2017/01/14 00:04:34]

Why ASCIIToUTF16 instead of base::UTF8ToUTF16?

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+                       date_created, date_last_used);
+}
+
 EntityDataPtr SpecificsToEntity(const AutofillSpecifics& specifics) {
   EntityData data;
   data.client_tag_hash = "ignored";
@@ -135,20 +160,38 @@ class AutocompleteSyncBridgeTest : public testing::Test {
       db_.Init(temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase"));
       backend_.SetWebDatabase(&db_);
 
+      sync_pb::ModelTypeState model_type_state;
+      table_.UpdateModelTypeState(syncer::AUTOFILL, model_type_state);

[skym, 2017/01/14 00:04:34]

Does this do anything? Do the tests pass if you take this out?

[Patrick Noland, 2017/01/17 22:59:55]

They all fail. LoadMetadata fails in the absence of stored ModelTypeState, which
reports an unexpected error, causing FakeModelTypeChangeProcessor to fail the
test. 

+
       bridge_.reset(new AutocompleteSyncBridge(
-          &backend_, base::Bind(&CreateModelTypeChangeProcessor)));
+          &backend_,
+          base::Bind(
+              &AutocompleteSyncBridgeTest::CreateModelTypeChangeProcessor,
+              base::Unretained(this))));
     }
   }
   ~AutocompleteSyncBridgeTest() override {}
 
- protected:
   AutocompleteSyncBridge* bridge() { return bridge_.get(); }

[maxbogue, 2017/01/14 00:39:02]

nit: we almost always have these sorts of accessors at the bottom of the public
section. just another little consistency thing.

[Patrick Noland, 2017/01/17 22:59:55]

Done.

 
+  syncer::RecordingModelTypeChangeProcessor* processor() { return processor_; }
+
+  AutofillTable* table() { return &table_; }
+
+  std::unique_ptr<syncer::ModelTypeChangeProcessor>
+  CreateModelTypeChangeProcessor(syncer::ModelType type,

[maxbogue, 2017/01/14 00:39:02]

this can be private

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+                                 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);
   }
@@ -221,9 +264,12 @@ class AutocompleteSyncBridgeTest : public testing::Test {
   ScopedTempDir temp_dir_;
   base::MessageLoop message_loop_;
   FakeAutofillBackend backend_;
-  AutofillTable table_;
   WebDatabase db_;
   std::unique_ptr<AutocompleteSyncBridge> bridge_;
+  // 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;
+  AutofillTable table_;

[maxbogue, 2017/01/14 00:39:02]

was there an ownership reason this one moved?

[Patrick Noland, 2017/01/17 22:59:55]

No, I'll put it back where it was. 

 
   DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest);
 };
@@ -426,4 +472,96 @@ TEST_F(AutocompleteSyncBridgeTest, ApplySyncChangesDatabaseFailure) {
   // making db calls and verify the errors are propagated up.
 }
 
+TEST_F(AutocompleteSyncBridgeTest, LocalEntriesAdded) {
+  const AutofillEntry added_entry1(

[maxbogue, 2017/01/14 00:39:02]

use = syntax (added_entry1 = CreateAutofillEntry...)

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      CreateAutofillEntryFromValues("added", "entry1", 1, 1));
+  const AutofillEntry added_entry2(

[maxbogue, 2017/01/14 00:39:02]

use = syntax

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      CreateAutofillEntryFromValues("added", "entry2", 1, 1));
+
+  table()->UpdateAutofillEntries({added_entry1, added_entry2});
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::ADD, added_entry1.key()),
+       AutofillChange(AutofillChange::ADD, added_entry2.key())});
+
+  EXPECT_EQ(2u, processor()->put_multimap().size());
+
+  const AutofillSpecifics specifics1 =
+      CreateAutofillSpecificsFromEntry(added_entry1);
+  const AutofillSpecifics specifics2 =
+      CreateAutofillSpecificsFromEntry(added_entry2);
+
+  const auto& recorded_specifics_for_key1 =
+      processor()->put_multimap().find(GetStorageKey(specifics1));
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key1);
+  AutofillSpecifics recorded_specifics =
+      recorded_specifics_for_key1->second->specifics.autofill();
+  VerifyEqual(recorded_specifics, specifics1);
+
+  const auto& recorded_specifics_for_key2 =
+      processor()->put_multimap().find(GetStorageKey(specifics2));
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key2);
+  recorded_specifics =
+      recorded_specifics_for_key2->second->specifics.autofill();
+  VerifyEqual(recorded_specifics, specifics2);
+}
+
+TEST_F(AutocompleteSyncBridgeTest, LocalEntryAddedThenUpdated) {
+  const AutofillEntry added_entry(

[maxbogue, 2017/01/14 00:39:02]

=

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      CreateAutofillEntryFromValues("my", "entry", 1, 1));
+  table()->UpdateAutofillEntries({added_entry});
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::ADD, added_entry.key())});
+
+  EXPECT_EQ(1u, processor()->put_multimap().size());
+
+  AutofillSpecifics specifics = CreateAutofillSpecificsFromEntry(added_entry);
+
+  const std::string storage_key = GetStorageKey(specifics);
+  auto recorded_specifics_for_key =
+      processor()->put_multimap().find(storage_key);
+  EXPECT_NE(processor()->put_multimap().end(), recorded_specifics_for_key);
+
+  AutofillSpecifics recorded_specifics =
+      recorded_specifics_for_key->second->specifics.autofill();
+  VerifyEqual(recorded_specifics, specifics);

[skym, 2017/01/14 00:04:34]

Seems like these couple lines keep repeating themselves. Can you create a helper
method to verify the put_multimap looks good?

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+
+  const AutofillEntry updated_entry(

[maxbogue, 2017/01/14 00:39:02]

=

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      CreateAutofillEntryFromValues("my", "entry", 1, 4));
+  table()->UpdateAutofillEntries({updated_entry});
+
+  specifics = CreateAutofillSpecificsFromEntry(updated_entry);
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::UPDATE, updated_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);
+
+  recorded_specifics = recorded_specifics_for_key->second->specifics.autofill();
+  VerifyEqual(specifics, recorded_specifics);
+}
+
+TEST_F(AutocompleteSyncBridgeTest, LocalEntryDeleted) {
+  const AutofillEntry deleted_entry(

[maxbogue, 2017/01/14 00:39:02]

=

[Patrick Noland, 2017/01/17 22:59:55]

Done.

+      CreateAutofillEntryFromValues("my", "entry", 1, 1));
+
+  const AutofillSpecifics specifics =
+      CreateAutofillSpecificsFromEntry(deleted_entry);
+  const std::string storage_key = GetStorageKey(specifics);
+
+  bridge()->AutofillEntriesChanged(
+      {AutofillChange(AutofillChange::REMOVE, deleted_entry.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());
+}
+
 }  // namespace autofill