[USS] Impelementation for GetData and GetAllData

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

+// 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) {
+  EXPECT_EQ(s1.SerializeAsString(), s2.SerializeAsString());
+}
+
+void VerifyDataBatch(std::map<std::string, AutofillSpecifics> expected,
+                     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/20 17:23:43]

What's the reason for const char* instead of const std::string&?

[Gang Wu, 2016/12/20 21:55:51]

function removed

+                                const char* value,
+                                int create_time_shift,

[skym, 2016/12/20 17:23:43]

What do you think of taking base::TimeDelta instead of ints?

[Gang Wu, 2016/12/20 21:55:50]

Done.

+                                int delete_time_shift) {

[skym, 2016/12/20 17:23:44]

Why is this called delete_time_shift? Isn't this the last used time?

[Gang Wu, 2016/12/20 21:55:50]

Done.

+  // Time deep in the past would cause Autocomplete sync to discard the
+  // entries.
+  static Time base_time = Time::Now().LocalMidnight();

[skym, 2016/12/20 17:23:44]

I didn't realize this was static. That actually is kinda nice. Can you add to
the comment explaining that this variable is static so that all autofill entries
are created with shifts relative to the same reference point in time.

[Gang Wu, 2016/12/20 21:55:50]

Done.

+
+  Time date_created = base_time + TimeDelta::FromSeconds(create_time_shift);

[skym, 2016/12/20 17:23:44]

FromSeconds is actually the wrong conversion, right?
base::Time::FromInternalValue is not converting to/from seconds.

[Gang Wu, 2016/12/20 21:55:50]

Done.

+  Time date_last_used = date_created;
+  if (delete_time_shift >= 0)

[skym, 2016/12/20 17:23:44]

Lets remove this >= 0 behavior completely and just trust our caller to give us a
reasonable second time.

[Gang Wu, 2016/12/20 21:55:51]

Done.

+    date_last_used = base_time + TimeDelta::FromSeconds(delete_time_shift);
+  return AutofillEntry(AutofillKey(ASCIIToUTF16(name), ASCIIToUTF16(value)),
+                       date_created, date_last_used);
+}
+
+AutofillEntry MakeAutofillEntry(const char* name,
+                                const char* value,
+                                int time_shift) {
+  return MakeAutofillEntry(name, value, time_shift, -1);

[skym, 2016/12/20 17:23:43]

I think passing -1 is bad. I was trying to suggest passing |time_shift| as both
the 3rd and 4th param to make it explicit that this overload is forcing both
creation time and last used time to be the same value.

[Gang Wu, 2016/12/20 21:55:50]

function removed.

+}
+
+std::unique_ptr<syncer::ModelTypeChangeProcessor>
+CreateModelTypeChangeProcessor(syncer::ModelType type,
+                               syncer::ModelTypeSyncBridge* bridge) {
+  auto processor = base::MakeUnique<syncer::FakeModelTypeChangeProcessor>();

[skym, 2016/12/20 17:23:43]

No reason for this function body to be two lines.

[Gang Wu, 2016/12/20 21:55:50]

Done.

+  return std::move(processor);
+}
+
+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 {}
+  void SetWebDatabase(WebDatabase* db) { db_ = db; }
+
+ private:
+  WebDatabase* db_;
+};
+
+}  // namespace
+
+class AutocompleteSyncBridgeTest : public testing::Test {
+ public:
+  AutocompleteSyncBridgeTest() : entry_(MakeAutofillEntry("0", "0", 0)) {}
+  ~AutocompleteSyncBridgeTest() override {}
+
+  AutocompleteSyncBridge* bridge() { return bridge_.get(); }

[skym, 2016/12/20 17:23:43]

This can be protected too, right?

[Gang Wu, 2016/12/20 21:55:51]

Done.

+
+ protected:
+  void SaveSpecificsToTable(const std::vector<AutofillSpecifics>& specifics) {
+    std::vector<AutofillEntry> new_entries;
+    for (const auto& specific : specifics) {

[skym, 2016/12/20 17:23:44]

Sorry to get really nit picky here, but I'm not happy with these variable names.
AutofillSpecifics has an 's' on the end of it. I'd prefer seeing something like

for (const auto& specifics : specifics_list) {

[Gang Wu, 2016/12/20 21:55:51]

Done.

+      new_entries.push_back(
+          AutocompleteSyncBridge::CreateAutofillEntry(specific));
+    }
+    table_.UpdateAutofillEntries(new_entries);
+  }
+
+  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 {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_ = temp_dir_.GetPath().AppendASCII("SyncTestWebDatabase");

[skym, 2016/12/20 17:23:44]

This can be in the ctor. It doesn't actually do file IO so we can can assert the
parent directory actually exists at a later time.

[Gang Wu, 2016/12/20 21:55:51]

https://cs.chromium.org/search/?q=CreateUniqueTempDir+file:%5Esrc/components/...
most of our unittest put this piece of code in the SetUp function, so for
consistence, I think we should do it in same way.
If you think we do need to put it in ctor, maybe we can do it in another CL
which move all the code into ctor.

[skym, 2016/12/20 22:38:05]

Looking at your search, there are a couple places that don't actually put
GetPath() in SetUp(). I'm not arguing to move CreateUnqiueTempDir(), I'm sold on
it being right here.

However, my previous suggestion was completely wrong. Looking at the
implementation, you cannot re-order CreateUniqueTempDir() and GetPath(), because
CreateUniqueTempDir() creates internal state in memory that GetPath() returns.

But, okay, new suggestion! You don't need to hang onto |file_|, right? You can
create it and pass it all inline when calling Init(..) on the DB.

[Gang Wu, 2016/12/22 01:33:53]

Done.

+
+    db_.AddTable(&table_);

[skym, 2016/12/20 17:23:44]

Why not move this into the ctor?

[Gang Wu, 2016/12/20 21:55:51]

same reason, autofill_table_unittest.cc:130, I just do the same way as autofill
table unittest did.

[skym, 2016/12/20 22:38:05]

See
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
, it seems to be generally better style to put it in the ctor if possible.

Though, splitting logic between the two is probably messier than putting
everything in SetUp(). Okay, I'm fine with your approach.

[Gang Wu, 2016/12/22 01:33:53]

Acknowledged.

+    ASSERT_EQ(sql::INIT_OK, db_.Init(file_));

[skym, 2016/12/20 17:23:44]

Probably out of scope of this CL, but we'll likely want to test the bridge's
code handling non-immediate initialization, which means this will probably have
to be in a separate method that test cases can delay calling.

[Gang Wu, 2016/12/20 21:55:50]

maybe cover in integration tests?

+    backend_.SetWebDatabase(&db_);
+
+    bridge_.reset(new AutocompleteSyncBridge(
+        &backend_, base::Bind(&CreateModelTypeChangeProcessor)));
+  }
+
+  void TearDown() override {}

[skym, 2016/12/20 17:23:43]

I'd just remove this until we need it.

[Gang Wu, 2016/12/20 21:55:50]

Done.

+
+  base::FilePath file_;

[skym, 2016/12/20 17:23:44]

These should all be private.

[Gang Wu, 2016/12/20 21:55:51]

Done.

+  base::MessageLoop message_loop_;
+  base::ScopedTempDir temp_dir_;
+  FakeAutofillBackend backend_;
+  AutofillTable table_;
+  WebDatabase db_;
+  std::unique_ptr<AutocompleteSyncBridge> bridge_;
+  AutofillEntry entry_;

[skym, 2016/12/20 17:23:44]

Still, why do you hang onto this AutofillEntry object?

[Gang Wu, 2016/12/20 21:55:50]

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, specifics2, specifics3});
+
+  const std::map<std::string, AutofillSpecifics> expected{
+      {GetStorageKey(specifics1), specifics1},
+      {GetStorageKey(specifics3), specifics3}};
+  bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics3)},
+                    base::Bind(&VerifyDataBatch, expected));
+}
+
+TEST_F(AutocompleteSyncBridgeTest, GetDataNotExist) {
+  const AutofillSpecifics specifics1 = CreateSpecifics(1);
+  const AutofillSpecifics specifics2 = CreateSpecifics(2);
+  const AutofillSpecifics specifics3 = CreateSpecifics(3);
+  SaveSpecificsToTable({specifics1, specifics2});
+
+  const std::map<std::string, AutofillSpecifics> expected{
+      {GetStorageKey(specifics1), specifics1},
+      {GetStorageKey(specifics2), specifics2}};
+  bridge()->GetData({GetStorageKey(specifics1), GetStorageKey(specifics2),
+                     GetStorageKey(specifics3)},
+                    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, specifics2, 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