Include a page title in the Physical Web omnibox overflow item

components/omnibox/browser/physical_web_provider_unittest.cc

Index: components/omnibox/browser/physical_web_provider_unittest.cc
diff --git a/components/omnibox/browser/physical_web_provider_unittest.cc b/components/omnibox/browser/physical_web_provider_unittest.cc
index d6f62fd17ac131b81c5b5d49a5108f7c6540b9b7..f20e4836b128cf48abc6940249c3dd9d03498b5e 100644
--- a/components/omnibox/browser/physical_web_provider_unittest.cc
+++ b/components/omnibox/browser/physical_web_provider_unittest.cc
@@ -20,6 +20,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/text_elider.h"
 #include "url/gurl.h"
 
 namespace {
@@ -84,6 +85,8 @@ class FakeAutocompleteProviderClient
   TestSchemeClassifier scheme_classifier_;
 };
 
+} // namespace
+
 class PhysicalWebProviderTest : public testing::Test {
  protected:
   PhysicalWebProviderTest() : provider_(NULL) {}
@@ -129,12 +132,109 @@ class PhysicalWebProviderTest : public testing::Test {
   // Construct an AutocompleteInput to represent tapping the omnibox with |url|
   // as the current web page.
   static AutocompleteInput CreateInputWithCurrentUrl(const std::string& url) {
-    return AutocompleteInput(base::ASCIIToUTF16(url), base::string16::npos,
+    return AutocompleteInput(base::UTF8ToUTF16(url), base::string16::npos,

[Mark P, 2016/09/13 23:34:43]

side comment: URLs are in ASCII as far as I know.  This change wasn't necessary.

[mattreynolds, 2016/09/14 19:04:41]

Acknowledged.

                              std::string(), GURL(url),
                              metrics::OmniboxEventProto::OTHER, false, false,
                              true, true, true, TestSchemeClassifier());
   }
 
+  // For a given |match|, check that the destination URL, contents string,
+  // description string, and default match state agree with the values specified
+  // in |url|, |contents|, |description|, and |allowed_to_be_default_match|.
+  static void ValidateMatch(const AutocompleteMatch& match,
+                            const std::string& url,
+                            const std::string& contents,
+                            const std::string& description,
+                            bool allowed_to_be_default_match) {
+    EXPECT_EQ(url, match.destination_url.spec());
+    EXPECT_EQ(contents, base::UTF16ToUTF8(match.contents));
+    EXPECT_EQ(description, base::UTF16ToUTF8(match.description));
+    EXPECT_EQ(allowed_to_be_default_match, match.allowed_to_be_default_match);
+  }
+
+  // Construct the contents string for an overflow item. Use |truncated_title|

[Mark P, 2016/09/13 23:34:43]

nit: Constructs -> Returns

[mattreynolds, 2016/09/14 19:04:41]

Done.

+  // as the title of the first match, |match_count_without_default| as the
+  // total number of matches (not counting the default match), and
+  // |metadata_count| as the number of nearby Physical Web URLs for which we
+  // have metadata.
+  static std::string ConstructOverflowItemContents(
+      const std::string& truncated_title,
+      size_t match_count_without_default,
+      size_t metadata_count) {
+    // Don't treat the overflow item as a metadata match.
+    const size_t metadata_match_count = match_count_without_default - 1;
+    // Determine how many URLs we didn't create match items for.
+    const size_t additional_url_count = metadata_count - metadata_match_count;
+
+    // Build the contents string.
+    if (truncated_title.empty()) {
+      return l10n_util::GetPluralStringFUTF8(
+          IDS_PHYSICAL_WEB_OVERFLOW_EMPTY_TITLE, additional_url_count);
+    } else {
+      // Subtract one from the additional URL count because the first item is
+      // represented by its title.
+      std::string contents_suffix = l10n_util::GetPluralStringFUTF8(
+          IDS_PHYSICAL_WEB_OVERFLOW, additional_url_count - 1);
+      return truncated_title + " " + contents_suffix;
+    }
+  }
+
+  // Run a test case using |input| as the simulated state of the omnibox input
+  // field, |metadata_list| as the list of simulated Physical Web metadata,
+  // and |title_truncated| as the truncated title of the first match. This will
+  // check for the presence of default matches and overflow items, as well as
+  // checking the fields of the overflow item. Metadata matches are not checked.
+  void OverflowItemTestCase(const AutocompleteInput& input,

[Mark P, 2016/09/13 23:34:43]

I'm a bit nervous about this function.  Rather than testing the output of the
match directly, it duplicates some of the logic (in particular the number of
item calculations) in the code to calculate what the output should look like.  I
think that code is correct, as do you, but if we're wrong, then this test case
is wrong and won't catch it.

[mattreynolds, 2016/09/14 19:04:41]

I removed the logic for computing the number of match items and now pass it as a
parameter.

+                            std::unique_ptr<base::ListValue> metadata_list,
+                            const std::string& title_truncated) {
+    const size_t metadata_count = metadata_list->GetSize();
+
+    const bool should_expect_default_match = !input.text().empty();
+
+    // The actual threshold for creating an overflow item may be lower than
+    // kMaxMatches, but for testing we always create enough metadata items to
+    // exceed the AutocompleteProvider's limit to ensure the overflow item is
+    // created.
+    const bool should_expect_overflow_item =
+        (metadata_count > AutocompleteProvider::kMaxMatches);
+
+    MockPhysicalWebDataSource* data_source =
+        client_->GetMockPhysicalWebDataSource();
+    EXPECT_TRUE(data_source);
+
+    data_source->SetMetadata(std::move(metadata_list));
+
+    provider_->Start(input, false);
+
+    const size_t match_count = provider_->matches().size();
+    const size_t match_count_without_default =
+        should_expect_default_match ? match_count - 1 : match_count;
+
+    if (should_expect_overflow_item) {
+      EXPECT_LT(match_count_without_default, metadata_count);
+    } else {
+      EXPECT_EQ(match_count_without_default, metadata_count);
+    }
+
+    size_t overflow_match_count = 0;
+    size_t default_match_count = 0;
+    for (const auto& match : provider_->matches()) {
+      if (match.type == AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW) {
+        std::string contents = ConstructOverflowItemContents(
+            title_truncated, match_count_without_default, metadata_count);
+        ValidateMatch(
+            match, "chrome://physical-web/", contents,
+            l10n_util::GetStringUTF8(IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION),
+            false);
+        ++overflow_match_count;
+      } else if (match.allowed_to_be_default_match) {
+        ++default_match_count;
+      }
+    }
+    EXPECT_EQ(should_expect_overflow_item ? 1U : 0U, overflow_match_count);
+    EXPECT_EQ(should_expect_default_match ? 1U : 0U, default_match_count);
+  }
+
   std::unique_ptr<FakeAutocompleteProviderClient> client_;
   scoped_refptr<PhysicalWebProvider> provider_;
 
@@ -182,10 +282,7 @@ TEST_F(PhysicalWebProviderTest, TestSingleMetadataItemCreatesOneMatch) {
   EXPECT_EQ(1U, provider_->matches().size());
   const AutocompleteMatch& metadata_match = provider_->matches().front();
   EXPECT_EQ(AutocompleteMatchType::PHYSICAL_WEB, metadata_match.type);
-  EXPECT_EQ(resolved_url, metadata_match.destination_url.spec());
-  EXPECT_EQ(resolved_url, base::UTF16ToASCII(metadata_match.contents));
-  EXPECT_EQ(title, base::UTF16ToASCII(metadata_match.description));
-  EXPECT_FALSE(metadata_match.allowed_to_be_default_match);
+  ValidateMatch(metadata_match, resolved_url, resolved_url, title, false);
 
   // Run the test again with a URL in the omnibox input. An additional match
   // should be added as a default match.
@@ -195,10 +292,7 @@ TEST_F(PhysicalWebProviderTest, TestSingleMetadataItemCreatesOneMatch) {
   size_t default_match_count = 0;
   for (const auto& match : provider_->matches()) {
     if (match.type == AutocompleteMatchType::PHYSICAL_WEB) {
-      EXPECT_EQ(resolved_url, match.destination_url.spec());
-      EXPECT_EQ(resolved_url, base::UTF16ToASCII(match.contents));
-      EXPECT_EQ(title, base::UTF16ToASCII(match.description));
-      EXPECT_FALSE(match.allowed_to_be_default_match);
+      ValidateMatch(match, resolved_url, resolved_url, title, false);
       ++metadata_match_count;
     } else {
       EXPECT_TRUE(match.allowed_to_be_default_match);
@@ -210,76 +304,6 @@ TEST_F(PhysicalWebProviderTest, TestSingleMetadataItemCreatesOneMatch) {
   EXPECT_EQ(1U, default_match_count);
 }
 
-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.
-  const size_t metadata_count = AutocompleteProvider::kMaxMatches + 1;
-
-  MockPhysicalWebDataSource* data_source =
-      client_->GetMockPhysicalWebDataSource();
-  EXPECT_TRUE(data_source);
-
-  data_source->SetMetadata(CreateMetadata(metadata_count));
-
-  {
-    // Run the test with no text in the omnibox input to simulate NTP.
-    provider_->Start(CreateInputForNTP(), false);
-
-    const size_t match_count = provider_->matches().size();
-    EXPECT_LT(match_count, metadata_count);
-
-    // Check that the overflow item is present and its fields are correct. There
-    // may be additional match items, but none should be marked as default.
-    size_t overflow_match_count = 0;
-    for (const auto& match : provider_->matches()) {
-      if (match.type == AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW) {
-        EXPECT_EQ("chrome://physical-web/", match.destination_url.spec());
-        EXPECT_EQ("chrome://physical-web/", base::UTF16ToASCII(match.contents));
-        const size_t metadata_matches = match_count - 1;
-        std::string description = l10n_util::GetPluralStringFUTF8(
-            IDS_PHYSICAL_WEB_OVERFLOW, metadata_count - metadata_matches);
-        EXPECT_EQ(description, base::UTF16ToASCII(match.description));
-        ++overflow_match_count;
-      }
-      EXPECT_FALSE(match.allowed_to_be_default_match);
-    }
-    EXPECT_EQ(1U, overflow_match_count);
-  }
-
-  {
-    // Run the test again with a URL in the omnibox input. An additional match
-    // should be added as a default match.
-    provider_->Start(CreateInputWithCurrentUrl("http://www.cnn.com"), false);
-
-    const size_t match_count = provider_->matches().size();
-    EXPECT_LT(match_count - 1, metadata_count);
-
-    // Check that the overflow item and default match are present and their
-    // fields are correct.
-    size_t overflow_match_count = 0;
-    size_t default_match_count = 0;
-    for (const auto& match : provider_->matches()) {
-      if (match.type == AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW) {
-        EXPECT_EQ("chrome://physical-web/", match.destination_url.spec());
-        EXPECT_EQ("chrome://physical-web/", base::UTF16ToASCII(match.contents));
-        const size_t metadata_matches = match_count - 2;
-        std::string description = l10n_util::GetPluralStringFUTF8(
-            IDS_PHYSICAL_WEB_OVERFLOW, metadata_count - metadata_matches);
-        EXPECT_EQ(description, base::UTF16ToASCII(match.description));
-        EXPECT_FALSE(match.allowed_to_be_default_match);
-        ++overflow_match_count;
-      } else if (match.allowed_to_be_default_match) {
-        ++default_match_count;
-      }
-    }
-    EXPECT_EQ(1U, overflow_match_count);
-    EXPECT_EQ(1U, default_match_count);
-  }
-}
-
 TEST_F(PhysicalWebProviderTest, TestNoMatchesWithUserInput) {
   MockPhysicalWebDataSource* data_source =
       client_->GetMockPhysicalWebDataSource();
@@ -290,8 +314,8 @@ TEST_F(PhysicalWebProviderTest, TestNoMatchesWithUserInput) {
   // 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(),
+  const AutocompleteInput input(
+      base::UTF8ToUTF16(text), text.length(), std::string(), GURL(),
       metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
       true, false, true, true, false, TestSchemeClassifier());
   provider_->Start(input, false);
@@ -299,4 +323,50 @@ TEST_F(PhysicalWebProviderTest, TestNoMatchesWithUserInput) {
   EXPECT_TRUE(provider_->matches().empty());
 }
 
+TEST_F(PhysicalWebProviderTest, TestManyMetadataItemsCreatesOverflowItem) {
+  // Create enough metadata to guarantee an overflow item will be created.
+  const size_t metadata_count = AutocompleteProvider::kMaxMatches + 1;
+
+  // Run the test with no text in the omnibox input to simulate NTP.
+  OverflowItemTestCase(CreateInputForNTP(), CreateMetadata(metadata_count),
+                       "Example title 0");
+
+  // Run the test again with a URL in the omnibox input. An additional match
+  // should be added as a default match.
+  OverflowItemTestCase(CreateInputWithCurrentUrl("http://www.cnn.com"),
+                       CreateMetadata(metadata_count), "Example title 0");
+}
+
+TEST_F(PhysicalWebProviderTest, TestLongPageTitleIsTruncatedInOverflowItem) {
+  // Set a long title for the first item. The page title for this item will
+  // appear in the overflow item's content string.
+  auto metadata_list = CreateMetadata(AutocompleteProvider::kMaxMatches + 1);
+  base::DictionaryValue* metadata_item;
+  EXPECT_TRUE(metadata_list->GetDictionary(0, &metadata_item));
+  metadata_item->SetString("title", "Extra long example title 0");
+
+  OverflowItemTestCase(CreateInputForNTP(), std::move(metadata_list),
+                       "Extra long exa" + std::string(gfx::kEllipsis));
+}
+
+TEST_F(PhysicalWebProviderTest, TestEmptyPageTitleInOverflowItem) {
+  // Set an empty title for the first item. Because the title is empty, we will
+  // display an alternate string in the overflow item's contents.
+  auto metadata_list = CreateMetadata(AutocompleteProvider::kMaxMatches + 1);
+  base::DictionaryValue* metadata_item;
+  EXPECT_TRUE(metadata_list->GetDictionary(0, &metadata_item));
+  metadata_item->SetString("title", "");
+
+  OverflowItemTestCase(CreateInputForNTP(), std::move(metadata_list), "");
+}
+
+TEST_F(PhysicalWebProviderTest, TestRTLPageTitleInOverflowItem) {
+  // Set a Hebrew title for the first item.
+  auto metadata_list = CreateMetadata(AutocompleteProvider::kMaxMatches + 1);
+  base::DictionaryValue* metadata_item;
+  EXPECT_TRUE(metadata_list->GetDictionary(0, &metadata_item));
+  metadata_item->SetString("title", "ויקיפדיה");
+
+  OverflowItemTestCase(CreateInputForNTP(), std::move(metadata_list),
+                       "ויקיפדיה");
 }