[omnibox] Don't generate document suggestions in explicit keyword mode

When keyword mode has been explicitly entered, and experimental keyword
mode has been enabled, we don't want document suggestions (unless its
search provider matches the keyword provider). Without this restriction,
these suggestions tend to show up when we're only expecting keyword
suggestions, since they rank highly.

Bug: 837395, 883901
Change-Id: If821f0efa8e3ffb6b05555ec6455849dc1d7f059
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1825779
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702485}

components/omnibox/browser/autocomplete_controller.cc

@@ -702,8 +702,8 @@ void AutocompleteController::UpdateKeywordDescriptions(
       i->description.clear();
       i->description_class.clear();
       DCHECK(!i->keyword.empty());
-      if ((i->keyword != last_keyword &&
-           !ShouldCurbKeywordDescriptions(i->keyword))) {
+      if (i->keyword != last_keyword &&
+          !ShouldCurbKeywordDescriptions(i->keyword)) {
         const TemplateURL* template_url =
             i->GetTemplateURL(template_url_service_, false);
         if (template_url) {

components/omnibox/browser/autocomplete_provider.cc

@@ -253,17 +253,24 @@ size_t AutocompleteProvider::TrimHttpPrefix(base::string16* url) {
 bool AutocompleteProvider::InExplicitExperimentalKeywordMode(
     const AutocompleteInput& input,
     const base::string16& keyword) {
+  return OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
+         input.prefer_keyword() &&
+         base::StartsWith(input.text(), keyword,
+                          base::CompareCase::SENSITIVE) &&
+         IsExplicitlyInKeywordMode(input, keyword);
+}
+
+// static
+bool AutocompleteProvider::IsExplicitlyInKeywordMode(
+    const AutocompleteInput& input,
+    const base::string16& keyword) {
   // It is important to this method that we determine if the user entered
   // keyword mode intentionally, as we use this routine to e.g. filter
   // all but keyword results. Currently we assume that the user entered
   // keyword mode intentionally with all entry methods except with a
   // space (and disregard entry method during a backspace). However, if the
   // user has typed a char past the space, we again assume keyword mode.
-  return OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
-         input.prefer_keyword() &&
-         base::StartsWith(input.text(), keyword,
-                          base::CompareCase::SENSITIVE) &&
-         (((input.keyword_mode_entry_method() !=
+  return (((input.keyword_mode_entry_method() !=
                 metrics::OmniboxEventProto::SPACE_AT_END &&
             input.keyword_mode_entry_method() !=
                 metrics::OmniboxEventProto::SPACE_IN_MIDDLE) &&

components/omnibox/browser/autocomplete_provider.h

@@ -289,6 +289,12 @@ class AutocompleteProvider
   static bool InExplicitExperimentalKeywordMode(const AutocompleteInput& input,
                                                 const base::string16& keyword);
 
+  // Uses the keyword entry mode in |input| (and possibly compare the length
+  // of the user input vs |keyword|) to decide if the user intentionally
+  // entered keyword mode.
+  static bool IsExplicitlyInKeywordMode(const AutocompleteInput& input,
+                                        const base::string16& keyword);
+
  protected:
   friend class base::RefCountedThreadSafe<AutocompleteProvider>;
   FRIEND_TEST_ALL_PREFIXES(BookmarkProviderTest, InlineAutocompletion);

components/omnibox/browser/document_provider.cc

@@ -37,6 +37,7 @@
 #include "components/omnibox/browser/document_suggestions_service.h"
 #include "components/omnibox/browser/history_provider.h"
 #include "components/omnibox/browser/in_memory_url_index_types.h"
+#include "components/omnibox/browser/keyword_provider.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/omnibox_pref_names.h"
 #include "components/omnibox/browser/search_provider.h"
@@ -356,7 +357,8 @@ void DocumentProvider::RegisterProfilePrefs(
 }
 
 bool DocumentProvider::IsDocumentProviderAllowed(
-    AutocompleteProviderClient* client) {
+    AutocompleteProviderClient* client,
+    const AutocompleteInput& input) {
   // Feature must be on.
   if (!base::FeatureList::IsEnabled(omnibox::kDocumentProvider))
     return false;
@@ -380,9 +382,8 @@ bool DocumentProvider::IsDocumentProviderAllowed(
     return false;
 
   // We haven't received a server backoff signal.
-  if (backoff_for_session_) {
+  if (backoff_for_session_)
     return false;
-  }
 
   // Google must be set as default search provider.
   auto* template_url_service = client->GetTemplateURLService();
@@ -390,9 +391,28 @@ bool DocumentProvider::IsDocumentProviderAllowed(
     return false;
   const TemplateURL* default_provider =
       template_url_service->GetDefaultSearchProvider();
-  return default_provider != nullptr &&
-         default_provider->GetEngineType(
-             template_url_service->search_terms_data()) == SEARCH_ENGINE_GOOGLE;
+  if (default_provider == nullptr ||
+      default_provider->GetEngineType(
+          template_url_service->search_terms_data()) != SEARCH_ENGINE_GOOGLE)
+    return false;
+  if (OmniboxFieldTrial::IsExperimentalKeywordModeEnabled() &&
+      input.prefer_keyword()) {
+    // If a keyword provider matches, and we're explicitly in keyword mode,
+    // then the keyword provider must match the default, or the document
+    // provider.
+    AutocompleteInput keyword_input = input;
+    const TemplateURL* keyword_provider =
+        KeywordProvider::GetSubstitutingTemplateURLForInput(
+            template_url_service, &keyword_input);
+    if (keyword_provider == nullptr)
+      return true;
+    // True if not explicitly in keyword mode, or a Drive suggestion.
+    return !IsExplicitlyInKeywordMode(input, keyword_provider->keyword()) ||
+           base::StartsWith(input.text(),
+                            base::ASCIIToUTF16("drive.google.com"),
+                            base::CompareCase::SENSITIVE);
+  }
+  return true;
 }
 
 // static
@@ -425,7 +445,7 @@ void DocumentProvider::Start(const AutocompleteInput& input,
 
   // Perform various checks - feature is enabled, user is allowed to use the
   // feature, we're not under backoff, etc.
-  if (!IsDocumentProviderAllowed(client_)) {
+  if (!IsDocumentProviderAllowed(client_, input)) {
     return;
   }
 

components/omnibox/browser/document_provider.h

@@ -85,6 +85,8 @@ class DocumentProvider : public AutocompleteProvider {
                            CheckFeaturePrerequisiteClientSettingOff);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
                            CheckFeaturePrerequisiteDefaultSearch);
+  FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
+                           CheckFeatureNotInExplicitKeywordMode);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
                            CheckFeaturePrerequisiteServerBackoff);
   FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, IsInputLikelyURL);
@@ -116,7 +118,8 @@ class DocumentProvider : public AutocompleteProvider {
 
   // Determines whether the profile/session/window meet the feature
   // prerequisites.
-  bool IsDocumentProviderAllowed(AutocompleteProviderClient* client);
+  bool IsDocumentProviderAllowed(AutocompleteProviderClient* client,
+                                 const AutocompleteInput& input);
 
   // Determines if the input is a URL, or is the start of the user entering one.
   // We avoid queries for these cases for quality and scaling reasons.

components/omnibox/browser/document_provider_unittest.cc

@@ -97,6 +97,22 @@ void DocumentProviderTest::SetUp() {
   default_template_url_ = turl_model->Add(std::make_unique<TemplateURL>(data));
   turl_model->SetUserSelectedDefaultSearchProvider(default_template_url_);
 
+  // Add a keyword provider.
+  data.SetShortName(base::ASCIIToUTF16("wiki"));
+  data.SetKeyword(base::ASCIIToUTF16("wikipedia.org"));
+  data.SetURL("https://en.wikipedia.org/w/index.php?search={searchTerms}");
+  data.suggestions_url =
+      "https://en.wikipedia.org/w/index.php?search={searchTerms}";
+  turl_model->Add(std::make_unique<TemplateURL>(data));
+
+  // Add another.
+  data.SetShortName(base::ASCIIToUTF16("drive"));
+  data.SetKeyword(base::ASCIIToUTF16("drive.google.com"));
+  data.SetURL("https://drive.google.com/drive/search?q={searchTerms}");
+  data.suggestions_url =
+      "https://drive.google.com/drive/search?q={searchTerms}";
+  turl_model->Add(std::make_unique<TemplateURL>(data));
+
   provider_ = DocumentProvider::Create(client_.get(), this, 4);
 }
 
@@ -107,7 +123,8 @@ void DocumentProviderTest::OnProviderUpdate(bool updated_matches) {
 TEST_F(DocumentProviderTest, CheckFeatureBehindFlag) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndDisableFeature(omnibox::kDocumentProvider);
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 }
 
 TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteNoIncognito) {
@@ -120,11 +137,13 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteNoIncognito) {
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
 
   // Feature starts enabled.
-  EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_TRUE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 
   // Feature should be disabled in incognito.
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(true));
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 }
 
 TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteNoSync) {
@@ -137,11 +156,13 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteNoSync) {
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
 
   // Feature starts enabled.
-  EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_TRUE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 
   // Feature should be disabled without active sync.
   EXPECT_CALL(*client_.get(), IsSyncActive()).WillOnce(Return(false));
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 }
 
 TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteClientSettingOff) {
@@ -154,12 +175,14 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteClientSettingOff) {
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
 
   // Feature starts enabled.
-  EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_TRUE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 
   // Disabling toggle in chrome://settings should be respected.
   PrefService* fake_prefs = client_->GetPrefs();
   fake_prefs->SetBoolean(omnibox::kDocumentSuggestEnabled, false);
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
   fake_prefs->SetBoolean(omnibox::kDocumentSuggestEnabled, true);
 }
 
@@ -173,7 +196,8 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteDefaultSearch) {
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
 
   // Feature starts enabled.
-  EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_TRUE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 
   // Switching default search disables it.
   TemplateURLService* template_url_service = client_->GetTemplateURLService();
@@ -185,12 +209,50 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteDefaultSearch) {
       template_url_service->Add(std::make_unique<TemplateURL>(data));
   template_url_service->SetUserSelectedDefaultSearchProvider(
       new_default_provider);
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
   template_url_service->SetUserSelectedDefaultSearchProvider(
       default_template_url_);
   template_url_service->Remove(new_default_provider);
 }
 
+TEST_F(DocumentProviderTest, CheckFeatureNotInExplicitKeywordMode) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      {omnibox::kDocumentProvider, omnibox::kExperimentalKeywordMode}, {});
+  EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*client_.get(), IsAuthenticated()).WillRepeatedly(Return(true));
+  EXPECT_CALL(*client_.get(), IsSyncActive()).WillRepeatedly(Return(true));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
+
+  // Prevent document search results in explicit keyword mode.
+  {
+    AutocompleteInput input(base::ASCIIToUTF16("wikipedia.org soup"),
+                            metrics::OmniboxEventProto::NTP,
+                            TestSchemeClassifier());
+    input.set_prefer_keyword(true);
+
+    EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get(), input));
+  }
+  {
+    AutocompleteInput input(base::ASCIIToUTF16("amazon.com soup"),
+                            metrics::OmniboxEventProto::NTP,
+                            TestSchemeClassifier());
+    input.set_prefer_keyword(true);
+
+    EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get(), input));
+  }
+  {
+    AutocompleteInput input(base::ASCIIToUTF16("drive.google.com soup"),
+                            metrics::OmniboxEventProto::NTP,
+                            TestSchemeClassifier());
+    input.set_prefer_keyword(true);
+
+    EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get(), input));
+  }
+}
+
 TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteServerBackoff) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(omnibox::kDocumentProvider);
@@ -201,11 +263,13 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteServerBackoff) {
   EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
 
   // Feature starts enabled.
-  EXPECT_TRUE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_TRUE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
 
   // Server setting backoff flag disables it.
   provider_->backoff_for_session_ = true;
-  EXPECT_FALSE(provider_->IsDocumentProviderAllowed(client_.get()));
+  EXPECT_FALSE(
+      provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
   provider_->backoff_for_session_ = false;
 }
 

components/omnibox/browser/search_provider.cc

@@ -1384,11 +1384,12 @@ int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {
 bool SearchProvider::ShouldCurbDefaultSuggestions() const {
   // Only curb if the global experimental keyword feature is enabled, we're
   // in keyword mode and we believe the user selected the mode explicitly.
-  if (providers_.has_keyword_provider())
+  if (providers_.has_keyword_provider()) {
     return InExplicitExperimentalKeywordMode(input_,
                                              providers_.keyword_provider());
-  else
+  } else {
     return false;
+  }
 }
 
 int SearchProvider::CalculateRelevanceForVerbatim() const {