Omnibox: Fix HistoryUrlProvider's match_in_foo implementation.

Fixes HistoryUrlProvider's match_in_foo implementation, and adds tests.

Bug: 732582
Change-Id: I5f63a74bb02a6cc97c771b26080217ea9559eb3b
Reviewed-on: https://chromium-review.googlesource.com/597170
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491804}

components/omnibox/browser/history_url_provider.cc

@@ -725,21 +725,25 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
         match.input_location = i->prefix.length();
         match.match_in_scheme = !i->num_components;
 
-        bool url_has_subdomain =
-            row_url.host_piece().length() >
+        size_t domain_length =
             net::registry_controlled_domains::GetDomainAndRegistry(
                 row_url.host_piece(),
                 net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)
                 .size();
-        bool input_matches_host =
-            row_url.host_piece().find(base::UTF16ToUTF8(
-                params->input.text())) != base::StringPiece::npos;
-        match.match_in_subdomain = url_has_subdomain && input_matches_host;
-
-        size_t path_pos =
-            row_url.parsed_for_possibly_invalid_spec().CountCharactersBefore(
-                url::Parsed::PATH, false);
-        match.match_after_host = prefixed_input.length() >= path_pos;
+        const url::Parsed& parsed = row_url.parsed_for_possibly_invalid_spec();
+
+        size_t host_pos =
+            parsed.CountCharactersBefore(url::Parsed::HOST, false);
+        size_t path_pos = parsed.CountCharactersBefore(url::Parsed::PATH, true);
+        size_t domain_pos = path_pos - domain_length;
+
+        // For the match to be in the subdomain, the prefix cannot encompass
+        // the subdomain, and the whole prefixed input (prefix + input) should
+        // be in the host or later.
+        match.match_in_subdomain = match.input_location < domain_pos &&
+                                   prefixed_input.length() > host_pos;
+
+        match.match_after_host = prefixed_input.length() > path_pos;
 
         match.innermost_match =
             i->num_components >= best_prefix->num_components;

components/omnibox/browser/history_url_provider_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/url_database.h"
@@ -52,7 +53,8 @@ struct TestURLInfo {
 } test_db[] = {
     {"http://www.google.com/", "Google", 3, 3, 80},
 
-  // High-quality pages should get a host synthesized as a lower-quality match.
+    // High-quality pages should get a host synthesized as a lower-quality
+    // match.
     {"http://slashdot.org/favorite_page.html", "Favorite page", 200, 100, 80},
 
     // Less popular pages should have hosts synthesized as higher-quality
@@ -67,8 +69,8 @@ struct TestURLInfo {
     {"http://news.google.com/", "Google News", 1, 1, 80},
 
     // Matches that are normally not inline-autocompletable should be
-  // autocompleted if they are shorter substitutes for longer matches that would
-  // have been inline autocompleted.
+    // autocompleted if they are shorter substitutes for longer matches that
+    // would have been inline autocompleted.
     {"http://synthesisatest.com/foo/", "Test A", 1, 1, 80},
     {"http://synthesisbtest.com/foo/", "Test B", 1, 1, 80},
     {"http://synthesisbtest.com/foo/bar.html", "Test B Bar", 2, 2, 80},
@@ -142,9 +144,9 @@ struct TestURLInfo {
     {"http://x.com/three", "Internet three", 2, 2, 80},
 
     // For punycode tests.
-  {"http://puny.xn--h2by8byc123p.in/", "Punycode", 2, 2, 5 },
+    {"http://puny.xn--h2by8byc123p.in/", "Punycode", 2, 2, 5},
     {"http://two_puny.xn--1lq90ic7f1rc.cn/",
-    "Punycode to be rendered in Unicode", 2, 2, 5 },
+     "Punycode to be rendered in Unicode", 2, 2, 5},
 
     // For experimental HUP scoring test.
     {"http://7.com/1a", "One", 8, 4, 4},
@@ -154,6 +156,10 @@ struct TestURLInfo {
     {"http://7.com/4a", "Four A", 1, 1, 32},
     {"http://7.com/4b", "Four B", 1, 1, 64},
     {"http://7.com/5a", "Five A", 8, 0, 64},  // never typed.
+
+    // For match URL formatting test.
+    {"https://www.abc.def.com/path", "URL with subdomain", 4, 4, 80},
+    {"https://www.hij.com/path", "URL with www only", 4, 4, 80},
 };
 
 class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
@@ -241,6 +247,11 @@ class HistoryURLProviderTest : public testing::Test,
                    expected_urls, num_results, &type);
   }
 
+  // Verifies that for the given |input_text|, the first match's contents
+  // are |expected_match_contents|.
+  void ExpectFormattedFullMatch(const std::string& input_text,
+                                const wchar_t* expected_match_contents);
+
   base::MessageLoop message_loop_;
   ACMatches matches_;
   std::unique_ptr<FakeAutocompleteProviderClient> client_;
@@ -351,6 +362,22 @@ void HistoryURLProviderTest::RunTest(
   }
 }
 
+void HistoryURLProviderTest::ExpectFormattedFullMatch(
+    const std::string& input_text,
+    const wchar_t* expected_match_contents) {
+  AutocompleteInput input(ASCIIToUTF16(input_text), base::string16::npos,
+                          std::string(), GURL(), base::string16(),
+                          metrics::OmniboxEventProto::INVALID_SPEC, false,
+                          false, true, true, false, TestSchemeClassifier());
+  autocomplete_->Start(input, false);
+  if (!autocomplete_->done())
+    base::RunLoop().Run();
+
+  // Test the variations of URL formatting on the first match.
+  EXPECT_EQ(base::WideToUTF16(expected_match_contents),
+            autocomplete_->matches().front().contents);
+}
+
 TEST_F(HistoryURLProviderTest, PromoteShorterURLs) {
   // Test that hosts get synthesized below popular pages.
   const UrlAndLegalDefault expected_nonsynth[] = {
@@ -1125,3 +1152,62 @@ TEST_F(HistoryURLProviderTest, HUPScoringExperiment) {
     }
   }
 }
+
+TEST_F(HistoryURLProviderTest, MatchURLFormatting) {
+  // Sanity check behavior under default flags.
+  ExpectFormattedFullMatch("abc", L"https://www.abc.def.com/path");
+  ExpectFormattedFullMatch("hij", L"https://www.hij.com/path");
+
+  auto feature_list = base::MakeUnique<base::test::ScopedFeatureList>();
+  feature_list->InitWithFeatures(
+      {omnibox::kUIExperimentHideSuggestionUrlScheme,
+       omnibox::kUIExperimentHideSuggestionUrlTrivialSubdomains,
+       omnibox::kUIExperimentElideSuggestionUrlAfterHost},
+      {});
+
+  // Sanity check that scheme, subdomain, and path can all be trimmed or elided.
+  ExpectFormattedFullMatch("hij", L"hij.com/\x2026\x0000");
+
+  // Verify that the scheme is preserved if part of match.
+  ExpectFormattedFullMatch("https://www.hi",
+                           L"https://www.hij.com/\x2026\x0000");
+
+  // Verify that the whole subdomain is preserved if part of match.
+  ExpectFormattedFullMatch("abc", L"www.abc.def.com/\x2026\x0000");
+  ExpectFormattedFullMatch("www.hij", L"www.hij.com/\x2026\x0000");
+
+  // Verify that the path is preserved if part of the match.
+  ExpectFormattedFullMatch("hij.com/pa", L"hij.com/path");
+
+  // Verify preserving both the scheme and subdomain.
+  ExpectFormattedFullMatch("https://www.hi",
+                           L"https://www.hij.com/\x2026\x0000");
+
+  // Verify preserving everything.
+  ExpectFormattedFullMatch("https://www.hij.com/p",
+                           L"https://www.hij.com/path");
+
+  // Verify that upper case input still works for subdomain matching.
+  ExpectFormattedFullMatch("WWW.hij", L"www.hij.com/\x2026\x0000");
+
+  // Verify that matching in the subdomain-only preserves the subdomain.
+  ExpectFormattedFullMatch("ww", L"www.abc.def.com/\x2026\x0000");
+  ExpectFormattedFullMatch("https://ww",
+                           L"https://www.abc.def.com/\x2026\x0000");
+
+  // Test individual feature flags as a sanity check.
+  feature_list.reset(new base::test::ScopedFeatureList);
+  feature_list->InitAndEnableFeature(
+      omnibox::kUIExperimentHideSuggestionUrlScheme);
+  ExpectFormattedFullMatch("hij", L"www.hij.com/path");
+
+  feature_list.reset(new base::test::ScopedFeatureList);
+  feature_list->InitAndEnableFeature(
+      omnibox::kUIExperimentHideSuggestionUrlTrivialSubdomains);
+  ExpectFormattedFullMatch("hij", L"https://hij.com/path");
+
+  feature_list.reset(new base::test::ScopedFeatureList);
+  feature_list->InitAndEnableFeature(
+      omnibox::kUIExperimentElideSuggestionUrlAfterHost);
+  ExpectFormattedFullMatch("hij", L"https://www.hij.com/\x2026\x0000");
+}