Include a page title in the Physical Web omnibox overflow item

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/string_number_conversions.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 "components/physical_web/data_source/physical_web_listener.h"
 #include "grit/components_strings.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 namespace {

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

didn't notice this the first time: should everything really be in an anonymous
namespace?  Why bother for a test file?

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

Fixed so only the mocks are in the anonymous 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()) {}
   ~MockPhysicalWebDataSource() override {}
 
   void StartDiscovery(bool network_request_enabled) override {}
   void StopDiscovery() override {}
@@ skipping 92 matching lines @@
 
   // 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,
                              std::string(), GURL(url),
                              metrics::OmniboxEventProto::OTHER, false, false,
                              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_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PhysicalWebProviderTest);
 };
 
 TEST_F(PhysicalWebProviderTest, TestEmptyMetadataListCreatesNoMatches) {
   MockPhysicalWebDataSource* data_source =
       client_->GetMockPhysicalWebDataSource();
@@ skipping 27 matching lines @@
 
   data_source->SetMetadata(std::move(metadata_list));
 
   // Run the test with no text in the omnibox input to simulate NTP.
   provider_->Start(CreateInputForNTP(), false);
 
   // Check that there is only one match item and its fields are correct.
   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.
   provider_->Start(CreateInputWithCurrentUrl("http://www.cnn.com"), false);
 
   size_t metadata_match_count = 0;
   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);
       ++default_match_count;
     }
   }
   EXPECT_EQ(2U, provider_->matches().size());
   EXPECT_EQ(1U, metadata_match_count);
   EXPECT_EQ(1U, default_match_count);
 }
@@ skipping 12 matching lines @@
 
   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);
 
+    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);
     }
     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);
 
+    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;
       }
     }
     EXPECT_EQ(1U, overflow_match_count);
     EXPECT_EQ(1U, default_match_count);
   }
 }
 
 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::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
       true, false, true, true, false, TestSchemeClassifier());
   provider_->Start(input, false);
 
   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);
 }
+
+}