[USS] Impelementation for GetData and GetAllData

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
new file mode 100644
index 0000000000000000000000000000000000000000..45ab8be0b88b2e907ff9990d8cf9c59d0f08c51c
--- /dev/null
+++ b/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
@@ -0,0 +1,241 @@
+// 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.
+
+#include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
+
+#include "base/bind.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.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/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/sync/model/data_batch.h"
+#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 "testing/gtest/include/gtest/gtest.h"
+
+using base::ASCIIToUTF16;
+using base::Time;
+using base::TimeDelta;
+using sync_pb::AutofillSpecifics;
+using syncer::SyncError;
+
+namespace autofill {
+
+namespace {
+
+const char kNameFormat[] = "name %d";
+const char kValueFormat[] = "value %d";
+
+void VerifyEqual(const AutofillSpecifics& s1, const AutofillSpecifics& s2) {

[skym, 2016/12/19 17:40:09]

Can you just call SerializeAsString() on both and make sure the strings are
equal? Sorry I set a bad example in device info sync bridge unittest!

[Gang Wu, 2016/12/19 23:28:10]

Done.

[skym, 2017/01/09 19:59:19]

I think I was wrong here to suggest this. Seeing two SerializeAsString() byte
arrays is not useful when you have a failing test case. Sorry!

+  EXPECT_EQ(s1.name(), s2.name());
+  EXPECT_EQ(s1.value(), s2.value());
+  EXPECT_EQ(Time::FromInternalValue(s1.usage_timestamp(0)),
+            Time::FromInternalValue(s2.usage_timestamp(0)));
+  EXPECT_EQ(s1.has_profile(), s2.has_profile());
+  if (s1.has_profile() && s2.has_profile()) {
+    EXPECT_EQ(s1.profile().guid(), s2.profile().guid());
+    EXPECT_EQ(s1.profile().origin(), s2.profile().origin());
+    EXPECT_EQ(s1.profile().use_count(), s2.profile().use_count());
+    EXPECT_EQ(s1.profile().use_date(), s2.profile().use_date());
+    EXPECT_EQ(s1.profile().name_first(0), s2.profile().name_first(0));
+    EXPECT_EQ(s1.profile().name_middle(0), s2.profile().name_middle(0));
+    EXPECT_EQ(s1.profile().name_last(0), s2.profile().name_last(0));
+    EXPECT_EQ(s1.profile().name_full(0), s2.profile().name_full(0));
+    EXPECT_EQ(s1.profile().email_address(0), s2.profile().email_address(0));
+    EXPECT_EQ(s1.profile().company_name(), s2.profile().company_name());
+    EXPECT_EQ(s1.profile().address_home_line1(),
+              s2.profile().address_home_line1());
+    EXPECT_EQ(s1.profile().address_home_line2(),
+              s2.profile().address_home_line2());
+    EXPECT_EQ(s1.profile().address_home_city(),
+              s2.profile().address_home_city());
+    EXPECT_EQ(s1.profile().address_home_state(),
+              s2.profile().address_home_state());
+    EXPECT_EQ(s1.profile().address_home_zip(), s2.profile().address_home_zip());
+    EXPECT_EQ(s1.profile().address_home_country(),
+              s2.profile().address_home_country());
+    EXPECT_EQ(s1.profile().address_home_street_address(),
+              s2.profile().address_home_street_address());
+    EXPECT_EQ(s1.profile().address_home_sorting_code(),
+              s2.profile().address_home_sorting_code());
+    EXPECT_EQ(s1.profile().address_home_dependent_locality(),
+              s2.profile().address_home_dependent_locality());
+    EXPECT_EQ(s1.profile().address_home_language_code(),
+              s2.profile().address_home_language_code());
+    EXPECT_EQ(s1.profile().phone_home_whole_number(0),
+              s2.profile().phone_home_whole_number(0));
+    EXPECT_EQ(s1.profile().guid(), s2.profile().guid());
+  }
+}
+
+void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected,

[skym, 2016/12/19 17:40:09]

Hmm, I wonder if we could make a templated version of this. Out of scope of this
CL though.

[Gang Wu, 2016/12/19 23:28:11]

Acknowledged.

+                     SyncError error,
+                     std::unique_ptr<syncer::DataBatch> batch) {
+  EXPECT_FALSE(error.IsSet());
+  while (batch->HasNext()) {
+    const syncer::KeyAndData& pair = batch->Next();
+    auto iter = expected.find(pair.first);
+    ASSERT_NE(iter, expected.end());
+    VerifyEqual(iter->second, pair.second->specifics.autofill());
+    // Removing allows us to verify we don't see the same item multiple times,
+    // and that we saw everything we expected.
+    expected.erase(iter);
+  }
+  EXPECT_TRUE(expected.empty());
+}
+
+AutofillEntry* MakeAutofillEntry(const char* name,

[skym, 2016/12/19 17:40:08]

Seems like a bunch of code is getting copied out of
profile_sync_service_autofill_unittest.cc. Would be ideal if we didn't duplicate
it.

Also, returning an owning raw ptr is probably not a great idea.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+                                 const char* value,
+                                 int time_shift0,

[skym, 2016/12/19 17:40:08]

time_shift<n> is a weird naming scheme.

Also, this defaulting logic is really confusing. What do you think of doing
something like the following instead:

ret foo1(arg1, arg2, arg3) { return foo2(arg1, arg2, arg3, arg3); }
ret foo2(arg1, arg2, arg3, arg4) { /* actual logic */ }

[Gang Wu, 2016/12/19 23:28:10]

Done.

+                                 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 new AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)),
+                           date_created, date_last_used);
+}
+
+class FakeAutofillBackend : public AutofillWebDataBackend {
+ public:
+  FakeAutofillBackend() {}
+  ~FakeAutofillBackend() override {}
+  WebDatabase* GetDatabase() override { return db_; }
+  void AddObserver(
+      autofill::AutofillWebDataServiceObserverOnDBThread* observer) override {}
+  void RemoveObserver(
+      autofill::AutofillWebDataServiceObserverOnDBThread* observer) override {}
+  void RemoveExpiredFormElements() override {}
+  void NotifyOfMultipleAutofillChanges() override {}
+  void NotifyThatSyncHasStarted(syncer::ModelType /* model_type */) override {}

[skym, 2016/12/19 17:40:09]

I didn't know you didn't have to name the variable. Cool. However, I'd remove
the commented out variable name. It's not adding anything.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+  void SetWebDatabase(WebDatabase* db) { db_ = db; }
+
+ private:
+  WebDatabase* db_;
+};
+
+class FakeWebDataService : public AutofillWebDataService {

[skym, 2016/12/19 17:40:08]

I'm probably missing something, but I don't understand why we need this. Can you
add some comments explaining how this works?

[Gang Wu, 2016/12/19 23:28:10]

Done.

+ public:
+  FakeWebDataService()
+      : AutofillWebDataService(base::ThreadTaskRunnerHandle::Get(),
+                               base::ThreadTaskRunnerHandle::Get()) {}
+
+ protected:
+  ~FakeWebDataService() override {}
+};
+
+}  // namespace
+
+class AutocompleteSyncBridgeTest : public testing::Test {
+ public:
+  AutocompleteSyncBridgeTest() {}
+  ~AutocompleteSyncBridgeTest() override {}
+
+  AutocompleteSyncBridge* Bridge() { return bridge_.get(); }

[skym, 2016/12/19 17:40:09]

I think I typically see this kind of function named bridge(), with a lowercase
b.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+
+ protected:
+  std::unique_ptr<syncer::ModelTypeChangeProcessor>
+  CreateModelTypeChangeProcessor(syncer::ModelType type,

[skym, 2016/12/19 17:40:09]

This method doesn't need to be an instanced method right now. Though this may be
short sighted.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+                                 syncer::ModelTypeSyncBridge* bridge) {
+    auto processor = base::MakeUnique<syncer::FakeModelTypeChangeProcessor>();
+    return std::move(processor);
+  }
+
+  void SaveSpecificsToTable(const AutofillSpecifics& specifics) {
+    std::vector<AutofillEntry> new_entries;
+    new_entries.push_back(
+        AutocompleteSyncBridge::CreateAutofillEntry(&specifics));
+    table_->UpdateAutofillEntries(new_entries);

[skym, 2016/12/19 17:40:08]

You can use C++11 list-initialization to inline the vector.

table_->UpdateAutofillEntries({AutocompleteSyncBridge::CreateAutofillEntry(&specifics)});

[Gang Wu, 2016/12/19 23:28:10]

Acknowledged.

+  }
+
+  AutofillSpecifics CreateSpecifics(int suffix) {
+    AutofillSpecifics specifics;
+    specifics.set_name(base::StringPrintf(kNameFormat, suffix));
+    specifics.set_value(base::StringPrintf(kValueFormat, suffix));
+    specifics.add_usage_timestamp(entry_->date_created().ToInternalValue());
+    return specifics;
+  }
+
+  std::string GetStorageKey(const AutofillSpecifics& specifics) {
+    return AutocompleteSyncBridge::FormatStorageKey(specifics.name(),
+                                                    specifics.value());
+  }
+
+  void SetUp() override {

[skym, 2016/12/19 17:40:08]

Seems to me like you could move most of the contents of this function into the
constructor.

[Gang Wu, 2016/12/19 23:28:11]

Done.

since assert cannot be in constructor, so I still need to leave something here.

+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase");
+
+    web_data_service_ = new FakeWebDataService();
+    table_.reset(new AutofillTable);

[skym, 2016/12/19 17:40:08]

table_ = base::MakeUnique<AutofillTable>();

[Gang Wu, 2016/12/19 23:28:10]

Done.

+    db_.reset(new WebDatabase);

[skym, 2016/12/19 17:40:08]

MakeUnique

[Gang Wu, 2016/12/19 23:28:10]

Done.

+    db_->AddTable(table_.get());
+    ASSERT_EQ(sql::INIT_OK, db_->Init(file_));
+    backend_.SetWebDatabase(db_.get());
+
+    entry_.reset(MakeAutofillEntry("0", "0", 0, -1));
+
+    bridge_.reset(new AutocompleteSyncBridge(
+        &backend_,
+        base::Bind(&AutocompleteSyncBridgeTest::CreateModelTypeChangeProcessor,
+                   base::Unretained(this))));
+  }
+
+  void TearDown() override {}
+
+  base::FilePath file_;
+  base::MessageLoop message_loop_;
+  base::ScopedTempDir temp_dir_;
+  FakeAutofillBackend backend_;
+  scoped_refptr<FakeWebDataService> web_data_service_;
+  std::unique_ptr<AutofillTable> table_;
+  std::unique_ptr<WebDatabase> db_;
+  std::unique_ptr<AutocompleteSyncBridge> bridge_;
+  std::unique_ptr<AutofillEntry> entry_;

[skym, 2016/12/19 17:40:09]

I don't like this entry instance field you're holding onto here. Lets make these
things on the stack as we need them.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncBridgeTest);
+};
+
+TEST_F(AutocompleteSyncBridgeTest, GetData) {
+  const AutofillSpecifics specifics1 = CreateSpecifics(1);
+  const AutofillSpecifics specifics2 = CreateSpecifics(2);
+  const AutofillSpecifics specifics3 = CreateSpecifics(3);
+  SaveSpecificsToTable(specifics1);

[skym, 2016/12/19 17:40:09]

Would it be nicer if you could pass a vector of specifics to save them?

const AutofillSpecifics specifics1 = CreateSpecifics(1);
const AutofillSpecifics specifics2 = CreateSpecifics(2);
const AutofillSpecifics specifics3 = CreateSpecifics(3);
SaveSpecificsToTable({specifics1, specifics2, specifics3});

Or maybe you could create them already saved

const AutofillSpecifics specifics1 = CreateSavedSpecifics(1);
const AutofillSpecifics specifics2 = CreateSavedSpecifics(2);
const AutofillSpecifics specifics3 = CreateSavedSpecifics(3);

[Gang Wu, 2016/12/19 23:28:10]

Done.

+  SaveSpecificsToTable(specifics2);
+  SaveSpecificsToTable(specifics3);
+
+  const std::map<std::string, AutofillSpecifics> expected{
+      {GetStorageKey(specifics1), specifics1},
+      {GetStorageKey(specifics3), specifics3}};
+  Bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics3)},

[skym, 2016/12/19 17:40:08]

You should also test calling GetData for a key that doesn't exist.

[Gang Wu, 2016/12/19 23:28:10]

Done.

+                    base::Bind(&VerifyDataBatch, expected));
+}
+
+TEST_F(AutocompleteSyncBridgeTest, GetAllData) {
+  const AutofillSpecifics specifics1 = CreateSpecifics(1);
+  const AutofillSpecifics specifics2 = CreateSpecifics(2);
+  const AutofillSpecifics specifics3 = CreateSpecifics(3);
+  SaveSpecificsToTable(specifics1);
+  SaveSpecificsToTable(specifics2);
+  SaveSpecificsToTable(specifics3);
+
+  const std::map<std::string, AutofillSpecifics> expected{
+      {GetStorageKey(specifics1), specifics1},
+      {GetStorageKey(specifics2), specifics2},
+      {GetStorageKey(specifics3), specifics3}};
+  Bridge()->GetAllData(base::Bind(&VerifyDataBatch, expected));
+}
+
+}  // namespace autofill