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..eed65762632c3de0f91b7528ae0f870736f018cd 100644
--- a/components/omnibox/browser/physical_web_provider_unittest.cc
+++ b/components/omnibox/browser/physical_web_provider_unittest.cc
@@ -135,6 +135,17 @@ class PhysicalWebProviderTest : public testing::Test {
                              true, true, true, TestSchemeClassifier());
   }
 
+  static void ValidateMatch(const AutocompleteMatch& match,

[Mark P, 2016/09/08 18:49:50]

nit: comment.

[mattreynolds, 2016/09/09 21:24:55]

Done.

+                            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::UTF16ToASCII(match.contents));
+    EXPECT_EQ(description, base::UTF16ToASCII(match.description));
+    EXPECT_EQ(allowed_to_be_default_match, match.allowed_to_be_default_match);
+  }
+
   std::unique_ptr<FakeAutocompleteProviderClient> client_;
   scoped_refptr<PhysicalWebProvider> provider_;
 
@@ -182,10 +193,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 +203,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);
@@ -231,17 +236,20 @@ TEST_F(PhysicalWebProviderTest, TestManyMetadataItemsCreatesOverflowItem) {
     const size_t match_count = provider_->matches().size();
     EXPECT_LT(match_count, metadata_count);
 
+    const size_t additional_url_count = metadata_count - match_count + 1;

[Mark P, 2016/09/08 18:49:50]

This is an unusual placement.  Either comment these are put them in the loop
below.  (The optimizing compiler is smart.)  Even putting them inline in the
ValidateMatch function call is fine with me.  I kept trying to figure out where
these variables were used outside the loop block.

ditto below

[mattreynolds, 2016/09/09 21:24:55]

Done.

+    std::string content = "Example title 0 " +
+        l10n_util::GetPluralStringFUTF8(IDS_PHYSICAL_WEB_OVERFLOW,
+                                        additional_url_count - 1);

[Mark P, 2016/09/08 18:49:50]

Also, I'm confused by all these +1 -1 here.  Is there a way you can write this
in a more straightforward manner?  For example, if you put the number
calculation inline here, you can simply do
metadata_count - match_count
with an appropriate comment
right?

Or perhaps just comment the declaration of additional_url_count better.  Right
now it's confusing, especially the +2 thing below.

[mattreynolds, 2016/09/09 21:24:55]

Yeah, this was pretty confusing. I added some comments and gave names to the
intermediate values to make it clearer.

+
     // 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));
+        ValidateMatch(
+            match, "chrome://physical-web/", content,
+            l10n_util::GetStringUTF8(IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION),
+            false);
         ++overflow_match_count;
       }
       EXPECT_FALSE(match.allowed_to_be_default_match);
@@ -257,19 +265,21 @@ TEST_F(PhysicalWebProviderTest, TestManyMetadataItemsCreatesOverflowItem) {
     const size_t match_count = provider_->matches().size();
     EXPECT_LT(match_count - 1, metadata_count);
 
+    const size_t additional_url_count = metadata_count - match_count + 2;
+    std::string content = "Example title 0 " +
+        l10n_util::GetPluralStringFUTF8(IDS_PHYSICAL_WEB_OVERFLOW,
+                                        additional_url_count - 1);
+
     // 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);
+        ValidateMatch(
+            match, "chrome://physical-web/", content,
+            l10n_util::GetStringUTF8(IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION),
+            false);
         ++overflow_match_count;
       } else if (match.allowed_to_be_default_match) {
         ++default_match_count;
@@ -299,4 +309,48 @@ TEST_F(PhysicalWebProviderTest, TestNoMatchesWithUserInput) {
   EXPECT_TRUE(provider_->matches().empty());
 }
 
+TEST_F(PhysicalWebProviderTest, TestLongPageTitleIsTruncated) {

[Mark P, 2016/09/08 18:49:50]

This test is only applicable for whether there's a lot of matches.  Either the
name or a comment should reflect that putting a lot of matches here is critical.
 Perhaps TestLongPageTitleIsTruncatedInOverflowItem?

You probably also want a comment somewhere that you don't truncate titles when
not in the overflow item -- you trust the omnibox display mechanism for
truncation that context.  You only worry about truncating when you
preferentially want to truncate something in the middle of the content string,
keeping the rest.

[mattreynolds, 2016/09/09 21:24:55]

Done. I added a note about truncation behavior in the kMaxTitleLength comment.

+  const size_t metadata_count = AutocompleteProvider::kMaxMatches + 1;
+
+  MockPhysicalWebDataSource* data_source =
+      client_->GetMockPhysicalWebDataSource();
+  EXPECT_TRUE(data_source);
+
+  auto metadata_list = CreateMetadata(metadata_count);
+
+  // Set a long title for the first item. The page title for this item will
+  // appear in the overflow item's content string.
+  base::DictionaryValue* metadata_item;
+  EXPECT_TRUE(metadata_list->GetDictionary(0, &metadata_item));
+  metadata_item->SetString("title", "Extra long example title 0");
+
+  data_source->SetMetadata(std::move(metadata_list));
+
+  // 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);
+
+  const size_t additional_url_count = metadata_count - match_count + 1;
+  std::string content = "Extra long exam... " +
+      l10n_util::GetPluralStringFUTF8(IDS_PHYSICAL_WEB_OVERFLOW,
+                                      additional_url_count - 1);
+
+  // 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) {
+      ValidateMatch(
+          match, "chrome://physical-web/", content,
+          l10n_util::GetStringUTF8(IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION),
+          false);
+      ++overflow_match_count;
+    }
+    EXPECT_FALSE(match.allowed_to_be_default_match);
+  }
+  EXPECT_EQ(1U, overflow_match_count);
+}
+
 }