Modified GetMatchToPrefetch() to return a valid match for prefetching even when hide_verbatim flag is turned off.

Details (repeating our email conversation) : This change is for re-implementation of Chrome Instant (Design doc: https://goto.google.com/fdgbe), in which the Suggest server tells Chrome which search suggestion to prefetch and then Chrome uses the hint details to prefetch the query results. We only prerender the top match marked for prefetching. But this doesn't work because of how Chrome deals with verbatim matches. So, we initially proposed that we require the search to be the top match, except for the cases where hide verbatim applied and the verbatim match is hidden. This works well but unfortunately, it seems like hide verbatim won't work without further UI changes in the omnibox. Until we figure out a way to hide the top verbatim match, we want to continue doing the same even with the hide verbatim turned off.

In this CL, I made changes to obey the prefetch hint if the search is the top match, unless it is only being outranked by a verbatim suggestion match.

BUG=269186
TEST=none

Review URL: https://codereview.chromium.org/69703002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@240747 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 181 insertions, 51 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_result.cc b/chrome/browser/autocomplete/autocomplete_result.cc
index 675a96c..f502edd 100644
--- a/chrome/browser/autocomplete/autocomplete_result.cc
+++ b/chrome/browser/autocomplete/autocomplete_result.cc
@@ -259,20 +259,15 @@ AutocompleteMatch* AutocompleteResult::match_at(size_t index) {
 
 bool AutocompleteResult::ShouldHideTopMatch() const {
   // Gate on our field trial flag.
-  if (!chrome::ShouldHideTopVerbatimMatch())
-    return false;
+  return chrome::ShouldHideTopVerbatimMatch() &&
+      TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches();
+}
 
-  // If we don't have a verbatim first match, show everything.
+bool AutocompleteResult::TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches()
+    const {
   if (empty() || !match_at(0).IsVerbatimType())
     return false;
-
-  // If the verbatim first match is followed by another verbatim match, don't
-  // hide anything, lest we cause user confusion.
-  if ((size() > 1) && match_at(1).IsVerbatimType())
-    return false;
-
-  // Otherwise, it's safe to hide the verbatim first match.
-  return true;
+  return !(size() > 1) || !match_at(1).IsVerbatimType();
 }
 
 void AutocompleteResult::Reset() {
diff --git a/chrome/browser/autocomplete/autocomplete_result.h b/chrome/browser/autocomplete/autocomplete_result.h
index 69bdbed..e7fec6c 100644
--- a/chrome/browser/autocomplete/autocomplete_result.h
+++ b/chrome/browser/autocomplete/autocomplete_result.h
@@ -125,6 +125,11 @@ class AutocompleteResult {
   // the dropdown should be closed.
   bool ShouldHideTopMatch() const;
 
+  // Returns true if the top match is a verbatim search or URL match (see
+  // IsVerbatimType() in autocomplete_match.h), and the next match is not also
+  // some kind of verbatim match.
+  bool TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches() const;
+
   const GURL& alternate_nav_url() const { return alternate_nav_url_; }
 
   // Clears the matches for this result set.
diff --git a/chrome/browser/autocomplete/autocomplete_result_unittest.cc b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
index e3fa9bc..cf1c1fd 100644
--- a/chrome/browser/autocomplete/autocomplete_result_unittest.cc
+++ b/chrome/browser/autocomplete/autocomplete_result_unittest.cc
@@ -22,6 +22,25 @@
 #include "components/variations/entropy_provider.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+// Creates an AutocompleteMatch using |destination_url| and |type| and appends
+// it to |matches|.
+void AddMatch(const std::string& destination_url, AutocompleteMatch::Type type,
+              ACMatches* matches) {
+  ASSERT_TRUE(matches != NULL);
+  AutocompleteMatch* last_match =
+    !matches->empty() ? &((*matches)[matches->size() - 1]) : NULL;
+  AutocompleteMatch match;
+  match.destination_url = GURL(destination_url);
+  match.relevance = last_match ? last_match->relevance - 100 : 1300;
+  match.allowed_to_be_default_match = true;
+  match.type = type;
+  matches->push_back(match);
+}
+
+}  // namespace
+
 class AutocompleteResultTest : public testing::Test  {
  public:
   struct TestData {
@@ -318,38 +337,16 @@ TEST_F(AutocompleteResultTest, SortAndCullDuplicateSearchURLs) {
 TEST_F(AutocompleteResultTest, SortAndCullWithDemotionsByType) {
   // Add some matches.
   ACMatches matches;
-  {
-    AutocompleteMatch match;
-    match.destination_url = GURL("http://history-url/");
-    match.relevance = 1400;
-    match.allowed_to_be_default_match = true;
-    match.type = AutocompleteMatchType::HISTORY_URL;
-    matches.push_back(match);
-  }
-  {
-    AutocompleteMatch match;
-    match.destination_url = GURL("http://search-what-you-typed/");
-    match.relevance = 1300;
-    match.allowed_to_be_default_match = true;
-    match.type = AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED;
-    matches.push_back(match);
-  }
-  {
-    AutocompleteMatch match;
-    match.destination_url = GURL("http://history-title/");
-    match.relevance = 1200;
-    match.allowed_to_be_default_match = true;
-    match.type = AutocompleteMatchType::HISTORY_TITLE;
-    matches.push_back(match);
-  }
-  {
-    AutocompleteMatch match;
-    match.destination_url = GURL("http://search-history/");
-    match.relevance = 500;
-    match.allowed_to_be_default_match = true;
-    match.type = AutocompleteMatchType::SEARCH_HISTORY;
-    matches.push_back(match);
-  }
+  AddMatch("http://history-url/", AutocompleteMatchType::HISTORY_URL, &matches);
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+
+  // Add a search history type match and demote its relevance score.
+  AddMatch("http://search-history/", AutocompleteMatchType::SEARCH_HISTORY,
+           &matches);
+  matches[matches.size() - 1].relevance = 500;
 
   // Add a rule demoting history-url and killing history-title.
   {
@@ -486,3 +483,131 @@ TEST_F(AutocompleteResultTest, SortAndCullReorderForDefaultMatch) {
     EXPECT_EQ("http://d/", result.match_at(3)->destination_url.spec());
   }
 }
+
+TEST_F(AutocompleteResultTest, ShouldHideTopMatch) {
+  // Add some matches.
+  ACMatches matches;
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+  AddMatch("http://search-history/", AutocompleteMatchType::SEARCH_HISTORY,
+           &matches);
+
+  base::FieldTrialList::CreateFieldTrial("InstantExtended",
+                                         "Group1 hide_verbatim:1");
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  EXPECT_TRUE(result.ShouldHideTopMatch());
+}
+
+TEST_F(AutocompleteResultTest, DoNotHideTopMatch) {
+  ACMatches matches;
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://url-what-you-typed/",
+           AutocompleteMatchType::URL_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+  AddMatch("http://search-history/", AutocompleteMatchType::SEARCH_HISTORY,
+           &matches);
+
+  base::FieldTrialList::CreateFieldTrial("InstantExtended",
+                                         "Group1 hide_verbatim:1");
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  // If the verbatim first match is followed by another verbatim match, don't
+  // hide the top verbatim match.
+  EXPECT_FALSE(result.ShouldHideTopMatch());
+}
+
+TEST_F(AutocompleteResultTest, DoNotHideTopMatch_TopMatchIsNotVerbatim) {
+  ACMatches matches;
+  AddMatch("http://search-history/", AutocompleteMatchType::SEARCH_HISTORY,
+           &matches);
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+
+  base::FieldTrialList::CreateFieldTrial("InstantExtended",
+                                         "Group1 hide_verbatim:1");
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  // Top match is not a verbatim type match. Do not hide the top match.
+  EXPECT_FALSE(result.ShouldHideTopMatch());
+}
+
+TEST_F(AutocompleteResultTest, DoNotHideTopMatch_FieldTrialFlagDisabled) {
+  // Add some matches. This test config is identical to ShouldHideTopMatch test
+  // except that the "hide_verbatim" flag is disabled in the field trials.
+  ACMatches matches;
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+  AddMatch("http://search-history/", AutocompleteMatchType::SEARCH_HISTORY,
+           &matches);
+
+  base::FieldTrialList::CreateFieldTrial("InstantExtended",
+                                         "Group1 hide_verbatim:0");
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  // Field trial flag "hide_verbatim" is disabled. Do not hide top match.
+  EXPECT_FALSE(result.ShouldHideTopMatch());
+}
+
+TEST_F(AutocompleteResultTest,
+       TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches) {
+  ACMatches matches;
+  AddMatch("http://url-what-you-typed/",
+           AutocompleteMatchType::URL_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  EXPECT_TRUE(result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches());
+}
+
+TEST_F(AutocompleteResultTest,
+       TopMatchIsVerbatimAndHasConsecutiveVerbatimMatches) {
+  ACMatches matches;
+  AddMatch("http://search-what-you-typed/",
+           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://url-what-you-typed/",
+           AutocompleteMatchType::URL_WHAT_YOU_TYPED, &matches);
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  EXPECT_FALSE(result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches());
+}
+
+TEST_F(AutocompleteResultTest, TopMatchIsNotVerbatim) {
+  ACMatches matches;
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+
+  // Result set is empty.
+  EXPECT_FALSE(result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches());
+
+  // Add a non-verbatim match to the result.
+  AddMatch("http://history-title/", AutocompleteMatchType::HISTORY_TITLE,
+           &matches);
+
+  result.AppendMatches(matches);
+  EXPECT_FALSE(result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches());
+}
+
+TEST_F(AutocompleteResultTest,
+       TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches_SingleMatchFound) {
+  ACMatches matches;
+  AddMatch("http://url-what-you-typed/",
+           AutocompleteMatchType::URL_WHAT_YOU_TYPED, &matches);
+
+  AutocompleteResult result;
+  result.AppendMatches(matches);
+  EXPECT_TRUE(result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches());
+}
diff --git a/chrome/browser/ui/omnibox/omnibox_controller.cc b/chrome/browser/ui/omnibox/omnibox_controller.cc
index 903f314..d488714 100644
--- a/chrome/browser/ui/omnibox/omnibox_controller.cc
+++ b/chrome/browser/ui/omnibox/omnibox_controller.cc
@@ -31,11 +31,14 @@ namespace {
 //
 // The SearchProvider may mark some suggestions to be prefetched based on
 // instructions from the suggest server. If such a match ranks sufficiently
-// highly, we'll return it. We only care about matches that are the default or
-// else the very first entry in the dropdown (which can happen for non-default
-// matches only if we're hiding a top verbatim match); for other matches, we
-// think the likelihood of the user selecting them is low enough that
-// prefetching isn't worth doing.
+// highly, we'll return it.
+//
+// We only care about matches that are the default or the very first entry in
+// the dropdown (which can happen for non-default matches only if we're hiding
+// a top verbatim match) or the second entry in the dropdown (which can happen
+// for non-default matches when a top verbatim match is shown); for other
+// matches, we think the likelihood of the user selecting them is low enough
+// that prefetching isn't worth doing.
 const AutocompleteMatch* GetMatchToPrefetch(const AutocompleteResult& result) {
   const AutocompleteResult::const_iterator default_match(
       result.default_match());
@@ -45,9 +48,11 @@ const AutocompleteMatch* GetMatchToPrefetch(const AutocompleteResult& result) {
   if (SearchProvider::ShouldPrefetch(*default_match))
     return &(*default_match);
 
-  return (result.ShouldHideTopMatch() && (result.size() > 1) &&
-      SearchProvider::ShouldPrefetch(result.match_at(1))) ?
-          &result.match_at(1) : NULL;
+  return ((result.ShouldHideTopMatch() ||
+              result.TopMatchIsVerbatimAndHasNoConsecutiveVerbatimMatches()) &&
+          (result.size() > 1) &&
+          SearchProvider::ShouldPrefetch(result.match_at(1))) ?
+              &result.match_at(1) : NULL;
 }
 
 }  // namespace