[omnibox] Try to convert all suggestions to tab switch suggestions

Currently we only attempt to convert history provider suggestions to
tab switch suggestions. This change moves the code to autocomplete
result and calls it from the controller after settling on the best
matches.

Also moves the unit test.

Bug: 780835
Change-Id: Ide8e7998911455766b8157d6dcbf8d65d32fd179
Reviewed-on: https://chromium-review.googlesource.com/1007482
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551765}

components/omnibox/browser/autocomplete_controller.cc

@@ -492,6 +492,9 @@ void AutocompleteController::UpdateResult(
   // Sort the matches and trim to a small number of "best" matches.
   result_.SortAndCull(input_, template_url_service_);
 
+  if (OmniboxFieldTrial::InTabSwitchSuggestionTrial())
+    result_.ConvertOpenTabMatches(provider_client_.get(), &input_);
+
   // Need to validate before invoking CopyOldMatches as the old matches are not
   // valid against the current input.
 #if DCHECK_IS_ON()

components/omnibox/browser/autocomplete_result.cc

@@ -18,12 +18,15 @@
 #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/autocomplete_provider_client.h"
 #include "components/omnibox/browser/match_compare.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/omnibox_switches.h"
+#include "components/strings/grit/components_strings.h"
 #include "components/url_formatter/url_fixer.h"
 #include "third_party/metrics_proto/omnibox_event.pb.h"
 #include "third_party/metrics_proto/omnibox_input_type.pb.h"
+#include "ui/base/l10n/l10n_util.h"
 
 // static
 size_t AutocompleteResult::GetMaxMatches() {
@@ -209,6 +212,44 @@ void AutocompleteResult::SortAndCull(
       GURL() : ComputeAlternateNavUrl(input, *default_match_);
 }
 
+void AutocompleteResult::ConvertOpenTabMatches(
+    AutocompleteProviderClient* client,
+    const AutocompleteInput* input) {
+  for (auto& match : matches_) {
+    // If already converted this match, don't re-search through open tabs and
+    // possibly re-change the description.
+    if (match.has_tab_match)
+      continue;
+    // If URL is not in a tab, there's nothing to do.
+    if (!client->IsTabOpenWithURL(match.destination_url, input))
+      continue;
+    match.has_tab_match = true;
+    // If we display a button for tab switching, don't change the description.
+    if (OmniboxFieldTrial::InTabSwitchSuggestionWithButtonTrial())
+      continue;
+    const base::string16 switch_tab_message =
+        l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT) +
+        base::UTF8ToUTF16(match.description.empty() ? "" : " - ");
+    match.description = switch_tab_message + match.description;
+    // Add classfication for the prefix.
+    if (match.description_class.empty()) {
+      match.description_class.push_back(
+          ACMatchClassification(0, ACMatchClassification::NONE));
+    } else {
+      if (match.description_class[0].style != ACMatchClassification::NONE) {
+        match.description_class.insert(
+            match.description_class.begin(),
+            ACMatchClassification(0, ACMatchClassification::NONE));
+      }
+      // Shift the rest.
+      for (auto& classification : match.description_class) {
+        if (classification.offset != 0 ||
+            classification.style != ACMatchClassification::NONE)
+          classification.offset += switch_tab_message.size();
+      }
+    }
+  }
+}
 bool AutocompleteResult::HasCopiedMatches() const {
   for (ACMatches::const_iterator i(begin()); i != end(); ++i) {
     if (i->from_previous)

components/omnibox/browser/autocomplete_result.h

@@ -17,6 +17,7 @@
 
 class AutocompleteInput;
 class AutocompleteProvider;
+class AutocompleteProviderClient;
 class TemplateURLService;
 
 // All matches from all providers for a particular query.  This also tracks
@@ -54,6 +55,13 @@ class AutocompleteResult {
   void SortAndCull(const AutocompleteInput& input,
                    TemplateURLService* template_url_service);
 
+  // Sets |has_tab_match| in matches whose URL matches an open tab's URL.
+  // Also, fixes up the description if not using another UI element to
+  // annotate (e.g. tab switch button). |input| can be null; if provided,
+  // the match can be more precise (e.g. scheme presence).
+  void ConvertOpenTabMatches(AutocompleteProviderClient* client,
+                             const AutocompleteInput* input);
+
   // Returns true if at least one match was copied from the last result.
   bool HasCopiedMatches() const;
 
@@ -121,6 +129,7 @@ class AutocompleteResult {
 
  private:
   friend class AutocompleteProviderTest;
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteResultTest, ConvertsOpenTabsCorrectly);
 
   typedef std::map<AutocompleteProvider*, ACMatches> ProviderToMatches;
 

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -15,11 +15,14 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
 #include "build/build_config.h"
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_match_type.h"
 #include "components/omnibox/browser/autocomplete_provider.h"
+#include "components/omnibox/browser/autocomplete_provider_client.h"
+#include "components/omnibox/browser/fake_autocomplete_provider_client.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/test_scheme_classifier.h"
 #include "components/search_engines/template_url_service.h"
@@ -118,6 +121,8 @@ class AutocompleteResultTest : public testing::Test {
     template_url_service_->Load();
   }
 
+  void TearDown() override { scoped_task_environment_.RunUntilIdle(); }
+
   // Configures |match| from |data|.
   void PopulateAutocompleteMatch(const TestData& data,
                                  AutocompleteMatch* match);
@@ -148,6 +153,7 @@ class AutocompleteResultTest : public testing::Test {
   std::unique_ptr<TemplateURLService> template_url_service_;
 
  private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
 
   // For every provider mentioned in TestData, we need a mock provider.
@@ -866,3 +872,31 @@ TEST_F(AutocompleteResultTest, InlineTailPrefixes) {
                                      cases[i].after_contents_class));
   }
 }
+
+TEST_F(AutocompleteResultTest, ConvertsOpenTabsCorrectly) {
+  AutocompleteResult result;
+  AutocompleteMatch match;
+  match.destination_url = GURL("http://this-site-matches.com");
+  result.matches_.push_back(match);
+  match.destination_url = GURL("http://other-site-matches.com");
+  match.description = base::UTF8ToUTF16("Some Other Site");
+  result.matches_.push_back(match);
+  match.destination_url = GURL("http://doesnt-match.com");
+  match.description = base::string16();
+  result.matches_.push_back(match);
+
+  // Have IsTabOpenWithURL() return true for some URLs.
+  FakeAutocompleteProviderClient client;
+  client.set_url_substring_match("matches");
+
+  result.ConvertOpenTabMatches(&client, nullptr);
+
+  EXPECT_TRUE(result.match_at(0)->has_tab_match);
+  EXPECT_EQ(base::UTF8ToUTF16("Switch to tab"),
+            result.match_at(0)->description);
+  EXPECT_TRUE(result.match_at(1)->has_tab_match);
+  EXPECT_EQ(base::UTF8ToUTF16("Switch to tab - Some Other Site"),
+            result.match_at(1)->description);
+  EXPECT_FALSE(result.match_at(2)->has_tab_match);
+  EXPECT_EQ(base::string16(), result.match_at(2)->description);
+}

components/omnibox/browser/fake_autocomplete_provider_client.cc

@@ -18,8 +18,7 @@
 #include "components/search_engines/template_url_service.h"
 
 FakeAutocompleteProviderClient::FakeAutocompleteProviderClient(
-    bool create_history_db)
-    : is_tab_open_with_url_(false) {
+    bool create_history_db) {
   set_template_url_service(std::make_unique<TemplateURLService>(nullptr, 0));
 
   bookmark_model_ = bookmarks::TestBookmarkClient::CreateModel();
@@ -84,5 +83,6 @@ FakeAutocompleteProviderClient::GetShortcutsBackendIfExists() {
 bool FakeAutocompleteProviderClient::IsTabOpenWithURL(
     const GURL& url,
     const AutocompleteInput* input) {
-  return is_tab_open_with_url_;
+  return !substring_to_match_.empty() &&
+         url.spec().find(substring_to_match_) != std::string::npos;
 }

components/omnibox/browser/fake_autocomplete_provider_client.h

@@ -51,8 +51,12 @@ class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
 
   bool IsTabOpenWithURL(const GURL& url,
                         const AutocompleteInput* input) override;
-  void set_is_tab_open_with_url(bool is_open) {
-    is_tab_open_with_url_ = is_open;
+
+  // A test calls this to establish the set of URLs that will return
+  // true from IsTabOpenWithURL() above. It's a simple substring match
+  // of the URL.
+  void set_url_substring_match(const std::string& substr) {
+    substring_to_match_ = substr;
   }
 
  private:
@@ -62,9 +66,11 @@ class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
   SearchTermsData search_terms_data_;
   std::unique_ptr<InMemoryURLIndex> in_memory_url_index_;
   std::unique_ptr<history::HistoryService> history_service_;
-  bool is_tab_open_with_url_;
   scoped_refptr<ShortcutsBackend> shortcuts_backend_;
 
+  // Substring used to match URLs for IsTabOpenWithURL().
+  std::string substring_to_match_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeAutocompleteProviderClient);
 };
 

components/omnibox/browser/history_provider.cc

@@ -7,16 +7,11 @@
 #include <string>
 
 #include "base/strings/string_util.h"
-#include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/in_memory_url_index_types.h"
-#include "components/omnibox/browser/omnibox_field_trial.h"
-#include "components/strings/grit/components_strings.h"
-#include "ui/base/l10n/l10n_util.h"
-#include "url/url_util.h"
 
 using bookmarks::BookmarkModel;
 
@@ -105,35 +100,3 @@ ACMatchClassifications HistoryProvider::SpansFromTermMatch(
 
   return spans;
 }
-
-void HistoryProvider::ConvertOpenTabMatches(const AutocompleteInput* input) {
-  for (auto& match : matches_) {
-    // If url is in a tab, change type, update classification.
-    if (client()->IsTabOpenWithURL(match.destination_url, input)) {
-      match.has_tab_match = true;
-      if (OmniboxFieldTrial::InTabSwitchSuggestionWithButtonTrial())
-        continue;
-      const base::string16 switch_tab_message =
-          l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT) +
-          base::UTF8ToUTF16(match.description.empty() ? "" : " - ");
-      match.description = switch_tab_message + match.description;
-      // Add classfication for the prefix.
-      if (match.description_class.empty()) {
-        match.description_class.push_back(
-            ACMatchClassification(0, ACMatchClassification::NONE));
-      } else {
-        if (match.description_class[0].style != ACMatchClassification::NONE) {
-          match.description_class.insert(
-              match.description_class.begin(),
-              ACMatchClassification(0, ACMatchClassification::NONE));
-        }
-        // Shift the rest.
-        for (auto& classification : match.description_class) {
-          if (classification.offset != 0 ||
-              classification.style != ACMatchClassification::NONE)
-            classification.offset += switch_tab_message.size();
-        }
-      }
-    }
-  }
-}

components/omnibox/browser/history_provider.h

@@ -45,14 +45,7 @@ class HistoryProvider : public AutocompleteProvider {
 
   AutocompleteProviderClient* client() { return client_; }
 
-  // Sets |has_tab_match| in matches whose URL matches an open tab's URL.
-  // Also, fixes up the description if not using another UI element to
-  // annotate (e.g. tab switch button). |input| can be null; if provided,
-  // the match can be more precise (e.g. scheme presence).
-  void ConvertOpenTabMatches(const AutocompleteInput* input);
-
  private:
-  FRIEND_TEST_ALL_PREFIXES(HistoryProviderTest, ConvertsOpenTabsCorrectly);
   AutocompleteProviderClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryProvider);

components/omnibox/browser/history_provider_unittest.cc

@@ -60,23 +60,9 @@ void HistoryProviderTest::TearDown() {
   scoped_task_environment_.RunUntilIdle();
 }
 
-}  // namespace
-
-TEST_F(HistoryProviderTest, ConvertsOpenTabsCorrectly) {
-  AutocompleteMatch match;
-  match.contents = base::UTF8ToUTF16("some-url.com");
-  provider()->matches_.push_back(match);
-  match.contents = base::UTF8ToUTF16("some-other-url.com");
-  match.description = base::UTF8ToUTF16("Some Other Site");
-  provider()->matches_.push_back(match);
-
-  // Have IsTabOpenWithURL() return true.
-  client()->set_is_tab_open_with_url(true);
-
-  provider()->ConvertOpenTabMatches(nullptr);
-
-  EXPECT_EQ(base::UTF8ToUTF16("Switch to tab"),
-            provider()->matches_[0].description);
-  EXPECT_EQ(base::UTF8ToUTF16("Switch to tab - Some Other Site"),
-            provider()->matches_[1].description);
+// Placeholder test. Remove after adding a substantive test.
+TEST_F(HistoryProviderTest, CreationTest) {
+  EXPECT_NE(client(), nullptr);
 }
+
+}  // namespace

components/omnibox/browser/history_quick_provider.cc

@@ -97,8 +97,6 @@ void HistoryQuickProvider::DoAutocomplete() {
     // Mark this max_match_score as being used.
     max_match_score--;
   }
-  if (OmniboxFieldTrial::InTabSwitchSuggestionTrial())
-    ConvertOpenTabMatches(&autocomplete_input_);
 }
 
 int HistoryQuickProvider::FindMaxMatchScore(

components/omnibox/browser/history_url_provider.cc

@@ -939,8 +939,6 @@ void HistoryURLProvider::QueryComplete(
       }
       matches_.push_back(HistoryMatchToACMatch(*params, i, relevance));
     }
-    if (OmniboxFieldTrial::InTabSwitchSuggestionTrial())
-      ConvertOpenTabMatches(&params->input);
   }
 
   done_ = true;