[omnibox] Have the builtin provider inline autocomplete more aggressively.

This is primarily motivated by the chrome://settings case, which
currently is never autocompleted because the builtin provider only
autocompletes in cases where there is a single suggestion. And any input
that matches chrome://settings also matches subpages such as
chrome://settings/autofill.

However, this change also affects one other current case,
chrome://chrome and chrome://chrome-urls. The former will now be inline
autocompleted.

Bug: 816450
Change-Id: I9671033c27c7f8bab1f7cfcbb2989e507e1aeee5
Reviewed-on: https://chromium-review.googlesource.com/976397
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550657}

components/omnibox/browser/builtin_provider.cc

@@ -120,15 +120,18 @@ void BuiltinProvider::Start(const AutocompleteInput& input,
     }
   }
 
+  // Provide a relevance score for each match.
   for (size_t i = 0; i < matches_.size(); ++i)
     matches_[i].relevance = kRelevance + matches_.size() - (i + 1);
+
+  // If allowing completions is okay and there's a match that's considered
+  // appropriate to be the default match, mark it as such and give it a high
+  // enough score to beat url-what-you-typed.
+  size_t default_match_index;
   if (!HistoryProvider::PreventInlineAutocomplete(input) &&
-      (matches_.size() == 1) && !matches_[0].inline_autocompletion.empty()) {
-    // If there's only one possible completion of the user's input and
-    // allowing completions is okay, give the match a high enough score to
-    // allow it to beat url-what-you-typed and be inlined.
-    matches_[0].relevance = 1250;
-    matches_[0].allowed_to_be_default_match = true;
+      HasMatchThatShouldBeDefault(&default_match_index)) {
+    matches_[default_match_index].relevance = 1250;
+    matches_[default_match_index].allowed_to_be_default_match = true;
   }
 }
 
@@ -146,3 +149,34 @@ void BuiltinProvider::AddMatch(const base::string16& match_string,
   match.contents_class = styles;
   matches_.push_back(match);
 }
+
+bool BuiltinProvider::HasMatchThatShouldBeDefault(size_t* index) const {
+  if (matches_.size() == 0)
+    return false;
+
+  // If there's only one possible completion of the user's input and it's not
+  // empty, it should be allowed to be the default match.
+  if (matches_.size() == 1) {
+    *index = 0;
+    return !matches_[0].inline_autocompletion.empty();
+  }
+
+  // If there's a non-empty completion that is a prefix of all of the others,
+  // it should be allowed to be the default match.
+  size_t shortest = 0;
+  for (size_t i = 1; i < matches_.size(); ++i) {
+    if (matches_[i].contents.length() < matches_[shortest].contents.length())
+      shortest = i;
+  }
+  if (matches_[shortest].inline_autocompletion.empty())
+    return false;
+
+  for (size_t i = 0; i < matches_.size(); ++i) {
+    if (!base::StartsWith(matches_[i].contents, matches_[shortest].contents,
+                          base::CompareCase::INSENSITIVE_ASCII)) {
+      return false;
+    }
+  }
+  *index = shortest;
+  return true;
+}

components/omnibox/browser/builtin_provider.h

@@ -35,6 +35,11 @@ class BuiltinProvider : public AutocompleteProvider {
                 const base::string16& inline_completion,
                 const ACMatchClassifications& styles);
 
+  // Returns true if |matches_| contains a match that should be allowed to be
+  // the default match. If true, the index of that match in |matches_| is
+  // returned in |index|.
+  bool HasMatchThatShouldBeDefault(size_t* index) const;
+
   AutocompleteProviderClient* client_;
   Builtins builtins_;
 

components/omnibox/browser/builtin_provider_unittest.cc

@@ -17,6 +17,7 @@
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_provider.h"
+#include "components/omnibox/browser/history_url_provider.h"
 #include "components/omnibox/browser/mock_autocomplete_provider_client.h"
 #include "components/omnibox/browser/test_scheme_classifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -56,8 +57,11 @@ class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
     std::vector<base::string16> urls;
     urls.push_back(ASCIIToUTF16(kHostBar));
     urls.push_back(ASCIIToUTF16(kHostMedia));
-    urls.push_back(ASCIIToUTF16(kHostMemory));
+    // The URL that is a superstring of the other is intentionally placed first
+    // here. The provider makes no guarantees that shorter URLs will appear
+    // first.
     urls.push_back(ASCIIToUTF16(kHostMemoryInternals));
+    urls.push_back(ASCIIToUTF16(kHostMemory));
     urls.push_back(ASCIIToUTF16(kHostSubpage));
 
     base::string16 prefix = ASCIIToUTF16(kHostSubpage) + ASCIIToUTF16("/");
@@ -109,12 +113,10 @@ class BuiltinProviderTest : public testing::Test {
       provider_->Start(input, false);
       EXPECT_TRUE(provider_->done());
       matches = provider_->matches();
-      EXPECT_EQ(cases[i].num_results, matches.size());
-      if (matches.size() == cases[i].num_results) {
-        for (size_t j = 0; j < cases[i].num_results; ++j) {
-          EXPECT_EQ(cases[i].output[j], matches[j].destination_url);
-          EXPECT_FALSE(matches[j].allowed_to_be_default_match);
-        }
+      ASSERT_EQ(cases[i].num_results, matches.size());
+      for (size_t j = 0; j < cases[i].num_results; ++j) {
+        EXPECT_EQ(cases[i].output[j], matches[j].destination_url);
+        EXPECT_FALSE(matches[j].allowed_to_be_default_match);
       }
     }
   }
@@ -202,8 +204,8 @@ TEST_F(BuiltinProviderTest, EmbedderProvidedURLs) {
   // The following hosts are arbitrary, chosen so that they all start with the
   // letters "me".
   const base::string16 kHostM1 = ASCIIToUTF16(kHostMedia);
-  const base::string16 kHostM2 = ASCIIToUTF16(kHostMemory);
-  const base::string16 kHostM3 = ASCIIToUTF16(kHostMemoryInternals);
+  const base::string16 kHostM2 = ASCIIToUTF16(kHostMemoryInternals);
+  const base::string16 kHostM3 = ASCIIToUTF16(kHostMemory);
   const GURL kURLM1(kEmbedder + kSep3 + kHostM1);
   const GURL kURLM2(kEmbedder + kSep3 + kHostM2);
   const GURL kURLM3(kEmbedder + kSep3 + kHostM3);
@@ -225,8 +227,8 @@ TEST_F(BuiltinProviderTest, EmbedderProvidedURLs) {
     {kAbout + kSep3 + kHostM1.substr(0, 3),    1, {kURLM1}},
     {kAbout + kSep3 + kHostM2.substr(0, 3),    2, {kURLM2, kURLM3}},
     {kAbout + kSep3 + kHostM1,                 1, {kURLM1}},
-    {kAbout + kSep2 + kHostM2,                 2, {kURLM2, kURLM3}},
-    {kAbout + kSep2 + kHostM3,                 1, {kURLM3}},
+    {kAbout + kSep2 + kHostM2,                 1, {kURLM2}},
+    {kAbout + kSep2 + kHostM3,                 2, {kURLM2, kURLM3}},
 
     // Typing an embedder URL should provide matching URLs.
     {kEmbedder + kSep1 + kHostM1.substr(0, 1), 3, {kURLM1, kURLM2, kURLM3}},
@@ -234,8 +236,8 @@ TEST_F(BuiltinProviderTest, EmbedderProvidedURLs) {
     {kEmbedder + kSep3 + kHostM1.substr(0, 3), 1, {kURLM1}},
     {kEmbedder + kSep3 + kHostM2.substr(0, 3), 2, {kURLM2, kURLM3}},
     {kEmbedder + kSep3 + kHostM1,              1, {kURLM1}},
-    {kEmbedder + kSep2 + kHostM2,              2, {kURLM2, kURLM3}},
-    {kEmbedder + kSep2 + kHostM3,              1, {kURLM3}},
+    {kEmbedder + kSep2 + kHostM2,              1, {kURLM2}},
+    {kEmbedder + kSep2 + kHostM3,              2, {kURLM2, kURLM3}},
   };
 
   RunTest(test_cases, arraysize(test_cases));
@@ -336,6 +338,12 @@ TEST_F(BuiltinProviderTest, Inlining) {
   const base::string16 kSep = ASCIIToUTF16(url::kStandardSchemeSeparator);
   const base::string16 kHostM = ASCIIToUTF16(kHostMedia);
   const base::string16 kHostB = ASCIIToUTF16(kHostBar);
+  const base::string16 kHostMem = ASCIIToUTF16(kHostMemory);
+  const base::string16 kHostMemInt = ASCIIToUTF16(kHostMemoryInternals);
+  const base::string16 kHostSub = ASCIIToUTF16(kHostSubpage);
+  const base::string16 kHostSubTwo = ASCIIToUTF16(kHostSubpage) +
+                                     ASCIIToUTF16("/") +
+                                     ASCIIToUTF16(kSubpageTwo);
 
   struct InliningTestData {
     const base::string16 input;
@@ -373,6 +381,43 @@ TEST_F(BuiltinProviderTest, Inlining) {
     {kEmbedder + kSep + kHostB.substr(0, 2), kHostB.substr(2)},
     {kEmbedder + kSep + kHostB.substr(0, 3), kHostB.substr(3)},
 
+    // The same rules should apply to "about://memory" and "chrome://memory".
+    // At the second "m", an inline autocompletion should be offered. Although
+    // this could also be completed with "memory-internals", "memory" is shorter
+    // and prefix of the other candidate, so it is preferred.
+    {kAbout + kSep + kHostMem.substr(0, 1), base::string16()},
+    {kAbout + kSep + kHostMem.substr(0, 2), base::string16()},
+    {kAbout + kSep + kHostMem.substr(0, 3), kHostMem.substr(3)},
+    {kAbout + kSep + kHostMem.substr(0, 4), kHostMem.substr(4)},
+    {kEmbedder + kSep + kHostMem.substr(0, 1), base::string16()},
+    {kEmbedder + kSep + kHostMem.substr(0, 2), base::string16()},
+    {kEmbedder + kSep + kHostMem.substr(0, 3), kHostMem.substr(3)},
+    {kEmbedder + kSep + kHostMem.substr(0, 4), kHostMem.substr(4)},
+
+    // After "memory-", then "memory-internals" should be inlined.
+    {kAbout + kSep + kHostMemInt.substr(0, 7), kHostMemInt.substr(7)},
+    {kEmbedder + kSep + kHostMemInt.substr(0, 7), kHostMemInt.substr(7)},
+
+    // Similarly, inline "about://subpage" and "chrome://subpage" even though
+    // other, longer completions (e.g. "chrome://subpage/one") are available.
+    {kAbout + kSep + kHostSub.substr(0, 1), kHostSub.substr(1)},
+    {kAbout + kSep + kHostSub.substr(0, 2), kHostSub.substr(2)},
+    {kAbout + kSep + kHostSub.substr(0, 3), kHostSub.substr(3)},
+    {kEmbedder + kSep + kHostSub.substr(0, 1), kHostSub.substr(1)},
+    {kEmbedder + kSep + kHostSub.substr(0, 2), kHostSub.substr(2)},
+    {kEmbedder + kSep + kHostSub.substr(0, 3), kHostSub.substr(3)},
+
+    // Once the user input distinctly matches a longer subpage
+    // ("chrome://subpage/two"), inline that. This doesn't happen until the user
+    // enters "w" so that it it can be distinguished from
+    // "chrome://subpage/three".
+    {kAbout + kSep + kHostSubTwo.substr(0, 8), base::string16()},
+    {kAbout + kSep + kHostSubTwo.substr(0, 9), base::string16()},
+    {kAbout + kSep + kHostSubTwo.substr(0, 10), kHostSubTwo.substr(10)},
+    {kEmbedder + kSep + kHostSubTwo.substr(0, 8), base::string16()},
+    {kEmbedder + kSep + kHostSubTwo.substr(0, 9), base::string16()},
+    {kEmbedder + kSep + kHostSubTwo.substr(0, 10), kHostSubTwo.substr(10)},
+
     // Typing something non-match after an inline autocompletion should stop
     // the inline autocompletion from appearing.
     {kAbout + kSep + kHostB.substr(0, 2) + ASCIIToUTF16("/"), base::string16()},
@@ -392,13 +437,33 @@ TEST_F(BuiltinProviderTest, Inlining) {
     if (cases[i].expected_inline_autocompletion.empty()) {
       // If we're not expecting an inline autocompletion, make sure that no
       // matches are allowed_to_be_default.
-      for (size_t j = 0; j < matches.size(); ++j)
+      for (size_t j = 0; j < matches.size(); ++j) {
+        EXPECT_LT(matches[j].relevance,
+                  HistoryURLProvider::kScoreForWhatYouTypedResult);
         EXPECT_FALSE(matches[j].allowed_to_be_default_match);
+      }
     } else {
+      // If we are expecting an inline autocompletion, confirm that one and only
+      // one of the matches is marked as allowed_to_be_default and that its
+      // inline autocompletion is equal to the expected inline autocompletion.
       ASSERT_FALSE(matches.empty());
-      EXPECT_TRUE(matches[0].allowed_to_be_default_match);
+      size_t default_match_index = matches.size();
+      for (size_t j = 0; j < matches.size(); ++j) {
+        // If we already found a match that is allowed_to_be_default, ensure
+        // that subsequent matches are NOT marked as allowed_to_be_default.
+        if (default_match_index < matches.size()) {
+          ASSERT_FALSE(matches[j].allowed_to_be_default_match)
+              << "Only one match should be allowed to be the default match.";
+        } else if (matches[j].allowed_to_be_default_match) {
+          default_match_index = j;
+        }
+      }
+      ASSERT_LT(default_match_index, matches.size())
+          << "One match should be marked as allowed to be default but none is.";
+      EXPECT_GT(matches[default_match_index].relevance,
+                HistoryURLProvider::kScoreForWhatYouTypedResult);
       EXPECT_EQ(cases[i].expected_inline_autocompletion,
-                matches[0].inline_autocompletion);
+                matches[default_match_index].inline_autocompletion);
     }
   }
 }