Add a Physical Web omnibox autocomplete provider

components/omnibox/browser/physical_web_provider_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/omnibox/browser/physical_web_provider.h"
+
+#include <memory>
+#include <string>
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "components/metrics/proto/omnibox_event.pb.h"
+#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
+#include "components/omnibox/browser/test_scheme_classifier.h"
+#include "components/physical_web/data_source/physical_web_data_source.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace {
+
+// A mock implementation of the Physical Web data source that allows setting
+// metadata for nearby URLs directly.
+class MockPhysicalWebDataSource : public PhysicalWebDataSource {
+ public:
+  MockPhysicalWebDataSource()
+      : metadata_(new base::ListValue())
+  {}

[Mark P, 2016/08/16 22:49:08]

nit: prefer { at end of previous line
ditto on the other constructors
(if everything fits on one line, you can put the {} on the same line;
if the constructor and initializer terms do not fit on one line, please
split the {} over two lines)

[mattreynolds, 2016/08/17 01:07:22]

Done.

+
+  ~MockPhysicalWebDataSource() override {}
+
+  // PhysicalWebDataSource implementation

[Mark P, 2016/08/16 22:49:09]

This comment makes it seem like it applies to only the block below, but it
applies to more things in this class.  Please revise for clarity or remove it.

[mattreynolds, 2016/08/17 01:07:22]

Removed

+  void StartDiscovery(bool network_request_enabled) override {}
+  void StopDiscovery() override {}
+
+  std::unique_ptr<base::ListValue> GetMetadata() override {
+    return metadata_->CreateDeepCopy();
+  }
+
+  bool HasUnresolvedDiscoveries() override {
+    return false;
+  }
+
+  // for testing
+  void SetMetadata(std::unique_ptr<base::ListValue> metadata) {
+    metadata_ = std::move(metadata);
+  }
+
+ private:
+  std::unique_ptr<base::ListValue> metadata_;
+};
+
+// An autocomplete provider client that embeds the mock Physical Web data
+// source.
+class FakeAutocompleteProviderClient
+    : public testing::NiceMock<MockAutocompleteProviderClient> {
+ public:
+  FakeAutocompleteProviderClient()
+     : physical_web_data_source_(base::MakeUnique<MockPhysicalWebDataSource>())
+  {}
+
+  const AutocompleteSchemeClassifier& GetSchemeClassifier() const override {
+    return scheme_classifier_;
+  }
+
+  PhysicalWebDataSource* GetPhysicalWebDataSource() override {
+    return physical_web_data_source_.get();
+  }
+
+  // Convenience method for accessing the mock data source.

[Mark P, 2016/08/16 22:49:08]

This seems unnecessary.

[mattreynolds, 2016/08/17 01:07:22]

Removed.

[Mark P, 2016/08/18 19:52:26]

Oops, I see why you had this--so you don't have to cast below.  Please put it
back (sorry!) and add something to the comment such as "so we don't have to
downcast in the code below."

[mattreynolds, 2016/08/18 22:33:32]

Done.

+  MockPhysicalWebDataSource* GetMockPhysicalWebDataSource() {
+    return physical_web_data_source_.get();
+  }
+
+ private:
+  std::unique_ptr<MockPhysicalWebDataSource> physical_web_data_source_;
+  TestSchemeClassifier scheme_classifier_;
+};
+
+class PhysicalWebProviderTest : public testing::Test {
+ protected:
+  PhysicalWebProviderTest() : provider_(NULL) {}
+  ~PhysicalWebProviderTest() override {}
+
+  void SetUp() override {
+    client_.reset(new FakeAutocompleteProviderClient());
+    provider_ = PhysicalWebProvider::Create(client_.get());
+  }
+
+  void TearDown() override {
+    provider_ = NULL;
+  }
+
+  static std::unique_ptr<base::ListValue> CreateMetadata(

[Mark P, 2016/08/16 22:49:08]

Comment.

[mattreynolds, 2016/08/17 01:07:22]

Done.

+      size_t metadata_count) {
+    auto metadata_list = base::MakeUnique<base::ListValue>();
+    for (size_t i = 0; i < metadata_count; ++i) {
+      std::ostringstream url;

[Mark P, 2016/08/16 22:49:08]

cleanup

[mattreynolds, 2016/08/17 01:07:22]

Not sure what this comment was referring to, the ostringstream is used below to
compose the url. This block was modified to add i to the title and description,
please take another look.

+      url << "https://example.com/" << i;
+      auto metadata_item = base::MakeUnique<base::DictionaryValue>();
+      metadata_item->SetString("scannedUrl", url.str());
+      metadata_item->SetString("resolvedUrl", url.str());
+      metadata_item->SetString("icon", url.str());
+      metadata_item->SetString("title", "Example title");

[Mark P, 2016/08/16 22:49:08]

consider putting "i" in the title and description as well

[mattreynolds, 2016/08/17 01:07:22]

Done.

+      metadata_item->SetString("description", "Example description");
+      metadata_list->Append(std::move(metadata_item));
+    }
+    return metadata_list;
+  }
+
+  std::unique_ptr<FakeAutocompleteProviderClient> client_;
+  scoped_refptr<PhysicalWebProvider> provider_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PhysicalWebProviderTest);
+};
+
+TEST_F(PhysicalWebProviderTest, TestEmptyMetadataListCreatesNoMatches) {
+  MockPhysicalWebDataSource* data_source =
+      client_->GetMockPhysicalWebDataSource();
+  EXPECT_TRUE(data_source);
+
+  data_source->SetMetadata(CreateMetadata(0));
+
+  // Construct an AutocompleteInput representing a typical "zero-suggest"
+  // case. The provider will verify that the content of the omnibox input field
+  // was not entered by the user.
+  std::string url("http://www.cnn.com/");
+  const AutocompleteInput input(base::ASCIIToUTF16(url), base::string16::npos,
+      std::string(), GURL(url), metrics::OmniboxEventProto::INVALID_SPEC,

[Mark P, 2016/08/16 22:49:08]

Prefer OTHER to be correspond with the http://www.cnn.com/ input.
ditto below

[mattreynolds, 2016/08/17 01:07:23]

Done.

+      true, false, true, true, true, TestSchemeClassifier());
+  provider_->Start(input, false);
+
+  EXPECT_TRUE(provider_->matches().empty());
+}
+
+TEST_F(PhysicalWebProviderTest, TestSingleMetadataItemCreatesOneMatch) {
+  MockPhysicalWebDataSource* data_source =
+      client_->GetMockPhysicalWebDataSource();
+  EXPECT_TRUE(data_source);
+
+  data_source->SetMetadata(CreateMetadata(1));
+
+  // Construct an AutocompleteInput representing a typical "zero-suggest"
+  // case. The provider will verify that the content of the omnibox input field
+  // was not entered by the user.
+  std::string url("http://www.cnn.com/");
+  const AutocompleteInput input(base::ASCIIToUTF16(url), base::string16::npos,
+      std::string(), GURL(url), metrics::OmniboxEventProto::INVALID_SPEC,
+      true, false, true, true, true, TestSchemeClassifier());
+  provider_->Start(input, false);
+
+  // Check that there is only one match item and it has the correct type.
+  EXPECT_TRUE(provider_->matches().size() == 1);

[Mark P, 2016/08/16 22:49:08]

Prefer
EXPECT_EQ(1, provider_->matches().size());
for better test failure messages.

Ditto elsewhere:
EXPECT_EQ(expected, actual)

There's also things like
EXPECT_LT, EXPECT_GT, EXPECT_GE, and so on.

[mattreynolds, 2016/08/17 01:07:22]

Done.

+  const AutocompleteMatch& metadata_match = provider_->matches().front();
+  EXPECT_TRUE(metadata_match.type == AutocompleteMatchType::PHYSICAL_WEB);

[Mark P, 2016/08/16 22:49:09]

Consider checking some of the other fields (i.e., URL goes to the right place,
title is set correctly, highlighting is correct, ...)

[mattreynolds, 2016/08/17 01:07:22]

Added checks for destination_url, contents, and description

+}
+
+TEST_F(PhysicalWebProviderTest, TestManyMetadataItemsCreatesOverflowItem) {
+  // This test is intended to verify that an overflow item is created when the
+  // number of nearby Physical Web URLs exceeds the maximum allowable matches
+  // for this provider. The actual limit for the PhysicalWebProvider may be
+  // changed in the future, so create enough metadata to exceed the
+  // AutocompleteProvider's limit.

[Mark P, 2016/08/16 22:49:08]

Good comment. :-)

+  const size_t metadata_count = AutocompleteProvider::kMaxMatches + 1;
+
+  MockPhysicalWebDataSource* data_source =
+      client_->GetMockPhysicalWebDataSource();
+  EXPECT_TRUE(data_source);
+
+  data_source->SetMetadata(CreateMetadata(metadata_count));
+
+  // Construct an AutocompleteInput representing a typical "zero-suggest"
+  // case. The provider will verify that the content of the omnibox input field
+  // was not entered by the user.
+  std::string url("http://www.cnn.com/");
+  const AutocompleteInput input(base::ASCIIToUTF16(url), base::string16::npos,
+      std::string(), GURL(url), metrics::OmniboxEventProto::INVALID_SPEC,
+      true, false, true, true, true, TestSchemeClassifier());
+  provider_->Start(input, false);
+
+  EXPECT_TRUE(provider_->matches().size() < metadata_count);
+
+  // Check that the overflow item is at the end of the match list and has the
+  // correct type.
+  const AutocompleteMatch& overflow_match = provider_->matches().back();
+  EXPECT_TRUE(
+      overflow_match.type == AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW);
+}
+
+TEST_F(PhysicalWebProviderTest, TestNoMatchesWithUserInput) {
+  MockPhysicalWebDataSource* data_source =
+      client_->GetMockPhysicalWebDataSource();
+  EXPECT_TRUE(data_source);
+
+  data_source->SetMetadata(CreateMetadata(1));
+
+  // Construct an AutocompleteInput to simulate user input in the omnibox input
+  // field. The provider should not generate any matches.
+  std::string text("user input");
+  const AutocompleteInput input(base::ASCIIToUTF16(text), text.length(),
+      std::string(), GURL(), metrics::OmniboxEventProto::INVALID_SPEC,

[Mark P, 2016/08/16 22:49:09]

consider using something like INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS instead
of INVALID_SPEC

[mattreynolds, 2016/08/17 01:07:22]

Done.

+      true, false, true, true, false, TestSchemeClassifier());
+  provider_->Start(input, false);
+
+  EXPECT_TRUE(provider_->matches().empty());
+}
+
+}