[Payments] Add web app manifest section table in SQLite web database

components/payments/android/web_app_manifest_section_table_unittest.cc

+// Copyright 2017 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/payments/android/web_app_manifest_section_table.h"
+
+#include "base/files/scoped_temp_dir.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace payments {
+namespace {
+
+const size_t kFingerPrintLength = 32;
+
+class WebAppManifestSectionTableTest : public testing::Test {
+ public:
+  WebAppManifestSectionTableTest() {}
+  ~WebAppManifestSectionTableTest() override {}
+
+ protected:
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_ = temp_dir_.GetPath().AppendASCII("TestWebDatabase");
+
+    table_.reset(new WebAppManifestSectionTable);
+    db_.reset(new WebDatabase);
+    db_->AddTable(table_.get());
+    ASSERT_EQ(sql::INIT_OK, db_->Init(file_));
+  }
+
+  void TearDown() override {}
+
+  base::FilePath file_;
+  base::ScopedTempDir temp_dir_;
+  std::unique_ptr<WebAppManifestSectionTable> table_;
+  std::unique_ptr<WebDatabase> db_;

[Scott Hess - ex-Googler, 2017/04/11 17:24:06]

Put this above |table_|, since |table_| might refer to |db_| (sorry if I
misunderstand this code).

[gogerald1, 2017/04/11 19:01:04]

we first create |table_| and then add it to |db_|, so it might make more sense
to keep |db_| below |table_|

[Scott Hess - ex-Googler, 2017/04/11 19:31:45]

The bit I'm worried about is that table_.db_ maintains a direct pointer to db_,
so could lead to use-after-free errors.

Though there is a comment on WebDatabaseTable::Db_ which indicates that this
case is expected, so I guess if this code considers that kind of thing
acceptable, I'll defer to previous owner reviews on the point.

[gogerald1, 2017/04/11 21:53:30]

Acknowledged. should be fine here since db_->Init(file_) will initialize
table_.db_. WebDataServiceWrapper will make sure db_ is initialized for the
table in production code.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WebAppManifestSectionTableTest);
+};
+
+TEST_F(WebAppManifestSectionTableTest, GetNonExistManifest) {
+  WebAppManifestSectionTable* web_app_manifest_section_table =
+      WebAppManifestSectionTable::FromWebDatabase(db_.get());
+  mojom::WebAppManifestSectionPtr retrieved_manifest =
+      web_app_manifest_section_table->GetWebAppManifest("https://bobpay.com");
+  ASSERT_TRUE(retrieved_manifest.get() == nullptr);
+}
+
+TEST_F(WebAppManifestSectionTableTest, AddAndGetManifest) {
+  // create a bobpay web app manifest.
+  mojom::WebAppManifestSectionPtr manifest =
+      mojom::WebAppManifestSection::New();
+  manifest->id = "com.bobpay";
+  manifest->min_version = static_cast<int64_t>(1);

[Scott Hess - ex-Googler, 2017/04/11 17:24:06]

I think 1l or 1L will also be the right 1.  Though I'm surprised simple 1
doesn't work.

[gogerald1, 2017/04/11 19:01:04]

Use "int64_t" would be more specific and understandable?

[Scott Hess - ex-Googler, 2017/04/11 19:31:45]

How would it be so?  Is it important to know in this context that min_version is
an int64_t?  We certainly don't annotate our other throw-away constants that
way, you weren't explicit with id (which is not a const char*).

[gogerald1, 2017/04/11 21:53:30]

The reason I was thinking is that: manifest->min_version is defined to int64_t
explicitly and 'L' doesn't mean 64 bit integer, make sense?

+  // Adds two finger prints.
+  for (size_t i = 0; i < 2U; i++) {
+    std::vector<uint8_t> fingerprint;
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      fingerprint.push_back(i * kFingerPrintLength + j);
+    }
+    manifest->fingerprints.push_back(fingerprint);
+  }

[Scott Hess - ex-Googler, 2017/04/11 17:24:06]

I see what you're doing, here, but it might also be worthwhile to just fill in
the fingerprints from a static buffer (or random data), then verify returned
value from the original buffer.  If you want to keep this form, then it might
make sense to have helpers for "generate from seed" and "compare from seed", so
that the duplicate bits of code don't get out of sync with each other.  Also,
the generate/compare code is complexity irrelevant to the test, this would read
cleaner as:
  manifest->id = ...;
  manifest->min_version = ...;
  manifest->fingerprints.push_back(GenerateFingerprint(...));
  manifest->fingerprints.push_back(GenerateFingerprint(...));

[gogerald1, 2017/04/11 19:01:04]

Done.

+
+  // Adds the manifest to the table.
+  WebAppManifestSectionTable* web_app_manifest_section_table =
+      WebAppManifestSectionTable::FromWebDatabase(db_.get());
+  ASSERT_TRUE(
+      web_app_manifest_section_table->AddWebAppManifest(manifest.get()));
+
+  // Gets and verifys the manifest.
+  mojom::WebAppManifestSectionPtr retrieved_manifest =
+      web_app_manifest_section_table->GetWebAppManifest("com.bobpay");
+  ASSERT_EQ(retrieved_manifest->id, "com.bobpay");
+  ASSERT_EQ(retrieved_manifest->min_version, 1);
+  ASSERT_EQ(retrieved_manifest->fingerprints.size(), 2U);
+  // Verify the two finger prints.
+  for (size_t i = 0; i < 2U; i++) {
+    ASSERT_EQ(retrieved_manifest->fingerprints[i].size(), kFingerPrintLength);
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      ASSERT_EQ(retrieved_manifest->fingerprints[i][j],
+                i * kFingerPrintLength + j);
+    }
+  }
+}
+
+TEST_F(WebAppManifestSectionTableTest, AddAndGetMultipleManifests) {
+  WebAppManifestSectionTable* web_app_manifest_section_table =
+      WebAppManifestSectionTable::FromWebDatabase(db_.get());
+
+  // Adds bobpay manifest to the table.
+  mojom::WebAppManifestSectionPtr manifest_1 =
+      mojom::WebAppManifestSection::New();
+  manifest_1->id = "com.bobpay";
+  manifest_1->min_version = static_cast<int64_t>(1);
+  // Adds two finger prints.
+  for (size_t i = 0; i < 2U; i++) {
+    std::vector<uint8_t> fingerprint;
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      fingerprint.push_back(i * kFingerPrintLength + j);
+    }
+    manifest_1->fingerprints.push_back(fingerprint);
+  }
+  ASSERT_TRUE(
+      web_app_manifest_section_table->AddWebAppManifest(manifest_1.get()));
+
+  // Adds alicepay manifest to the table.
+  mojom::WebAppManifestSectionPtr manifest_2 =
+      mojom::WebAppManifestSection::New();
+  manifest_2->id = "com.alicepay";
+  manifest_2->min_version = static_cast<int64_t>(2);
+  // Adds two finger prints.
+  for (size_t i = 0; i < 2U; i++) {
+    std::vector<uint8_t> fingerprint;
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      fingerprint.push_back(2 * i * kFingerPrintLength + j);
+    }
+    manifest_2->fingerprints.push_back(fingerprint);
+  }
+  ASSERT_TRUE(
+      web_app_manifest_section_table->AddWebAppManifest(manifest_2.get()));
+
+  // Verifys bobpay manifest.
+  mojom::WebAppManifestSectionPtr bobpay_manifest =
+      web_app_manifest_section_table->GetWebAppManifest("com.bobpay");
+  ASSERT_EQ(bobpay_manifest->id, "com.bobpay");
+  ASSERT_EQ(bobpay_manifest->min_version, 1);
+  ASSERT_EQ(bobpay_manifest->fingerprints.size(), 2U);
+  for (size_t i = 0; i < 2U; i++) {
+    ASSERT_EQ(bobpay_manifest->fingerprints[i].size(), kFingerPrintLength);
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      ASSERT_EQ(bobpay_manifest->fingerprints[i][j],
+                i * kFingerPrintLength + j);
+    }
+  }
+
+  // Verifys alicepay manifest.
+  mojom::WebAppManifestSectionPtr alicepay_manifest =
+      web_app_manifest_section_table->GetWebAppManifest("com.alicepay");
+  ASSERT_EQ(alicepay_manifest->id, "com.alicepay");
+  ASSERT_EQ(alicepay_manifest->min_version, 2);
+  ASSERT_EQ(alicepay_manifest->fingerprints.size(), 2U);
+  for (size_t i = 0; i < 2U; i++) {
+    ASSERT_EQ(alicepay_manifest->fingerprints[i].size(), kFingerPrintLength);
+    for (size_t j = 0; j < kFingerPrintLength; j++) {
+      ASSERT_EQ(alicepay_manifest->fingerprints[i][j],
+                2 * i * kFingerPrintLength + j);
+    }
+  }
+}
+}
+}  // payments