[omnibox] improve on device head suggest incognito flags.

1. Allows to enable on device head suggest in incognito w/o enabling it
in non incognito first.
2. Removes request delay in incognito since there is no server
suggestion which we need to cope with.

Bug: 925072
Change-Id: I344ecc2c620cb95c3b5f7c929cac0d9287b3c05b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881978Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Ce Chen <cch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710127}

components/omnibox/browser/omnibox_field_trial.cc

@@ -681,11 +681,6 @@ bool OmniboxFieldTrial::IsMaxURLMatchesFeatureEnabled() {
   return base::FeatureList::IsEnabled(omnibox::kOmniboxMaxURLMatches);
 }
 
-bool OmniboxFieldTrial::IsOnDeviceHeadProviderEnabledForIncognito() {
-  return base::GetFieldTrialParamByFeatureAsBool(omnibox::kOnDeviceHeadProvider,
-                                                 "EnableForIncongnito", false);
-}
-
 const char OmniboxFieldTrial::kBundledExperimentFieldTrialName[] =
     "OmniboxBundledExperimentV1";
 const char OmniboxFieldTrial::kDisableProvidersRule[] = "DisableProviders";

components/omnibox/browser/omnibox_field_trial.h

@@ -414,9 +414,6 @@ bool IsGroupSuggestionsBySearchVsUrlFeatureEnabled();
 // is enabled.
 bool IsMaxURLMatchesFeatureEnabled();
 
-// Returns whether on device head provider is enabled for incognito mode.
-bool IsOnDeviceHeadProviderEnabledForIncognito();
-
 // ---------------------------------------------------------
 // Clipboard URL suggestions:
 

components/omnibox/browser/on_device_head_provider.cc

@@ -108,11 +108,20 @@ bool OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed(
   if (!client()->SearchSuggestEnabled())
     return false;
 
-  // Make sure user is not in incognito, unless on device head provider is
-  // enabled for incognito.
-  if (client()->IsOffTheRecord() &&
-      !OmniboxFieldTrial::IsOnDeviceHeadProviderEnabledForIncognito())
+  // This flag specifies whether we should serve incognito or non incognito
+  // request, value can be:
+  // 1. incognito-not-allowed (or empty string): only serve non incognito
+  //    request; this is the default behavior;
+  // 2. incognito-only: only serve incognito request;
+  // 3. always-serve: always serve regardless of incognito.
+  const std::string mode = base::GetFieldTrialParamValueByFeature(
+      omnibox::kOnDeviceHeadProvider, "IncognitoServeMode");
+  if (mode != "always-serve") {
+    if (client()->IsOffTheRecord() && mode != "incognito-only")
       return false;
+    if (!client()->IsOffTheRecord() && mode == "incognito-only")
+      return false;
+  }
 
   // Reject on focus request.
   if (input.from_omnibox_focus())
@@ -156,8 +165,14 @@ void OnDeviceHeadProvider::Start(const AutocompleteInput& input,
     // from server, if we issue both requests simultaneously.
     // Therefore, we might want to delay the On Device suggest requests (and
     // also apply a timeout to search default loader) to mitigate this issue.
-    int delay = base::GetFieldTrialParamByFeatureAsInt(
-        omnibox::kOnDeviceHeadProvider, "DelayOnDeviceHeadSuggestRequestMs", 0);
+    // Note this delay is not needed for incognito where server suggestion is
+    // not served.
+    int delay = 0;
+    if (!client()->IsOffTheRecord()) {
+      delay = base::GetFieldTrialParamByFeatureAsInt(
+          omnibox::kOnDeviceHeadProvider, "DelayOnDeviceHeadSuggestRequestMs",
+          0);
+    }
     task_runner_->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&OnDeviceHeadProvider::DoSearch,

components/omnibox/browser/on_device_head_provider_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "build/build_config.h"
 #include "components/omnibox/browser/autocomplete_input.h"
@@ -15,6 +16,7 @@
 #include "components/omnibox/browser/fake_autocomplete_provider_client.h"
 #include "components/omnibox/browser/on_device_head_serving.h"
 #include "components/omnibox/browser/test_scheme_classifier.h"
+#include "components/omnibox/common/omnibox_features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -75,7 +77,7 @@ TEST_F(OnDeviceHeadProviderTest, ServingInstanceNotCreated) {
   input.set_want_asynchronous_matches(true);
   ResetServingInstance();
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
   EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
 
   provider_->Start(input, false);
@@ -100,21 +102,61 @@ TEST_F(OnDeviceHeadProviderTest, RejectSynchronousRequest) {
   EXPECT_TRUE(provider_->done());
 }
 
-TEST_F(OnDeviceHeadProviderTest, RejectIncognito) {
+TEST_F(OnDeviceHeadProviderTest, TestIncognito) {
   AutocompleteInput input(base::UTF8ToUTF16("M"),
                           metrics::OmniboxEventProto::OTHER,
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(true));
-  EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(true));
+  EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
+      .WillRepeatedly(Return(true));
 
+  // By default incognito request will be rejected.
+  {
     provider_->Start(input, false);
     if (!provider_->done())
       task_environment_.RunUntilIdle();
 
     EXPECT_TRUE(provider_->matches().empty());
     EXPECT_TRUE(provider_->done());
+  }
+
+  // Now enable for incognito only.
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kOnDeviceHeadProvider,
+        {{"IncognitoServeMode", "incognito-only"}});
+
+    provider_->Start(input, false);
+    if (!provider_->done())
+      task_environment_.RunUntilIdle();
+
+    EXPECT_TRUE(provider_->done());
+    ASSERT_EQ(3U, provider_->matches().size());
+    EXPECT_EQ(base::UTF8ToUTF16("maps"), provider_->matches()[0].contents);
+    EXPECT_EQ(base::UTF8ToUTF16("mail"), provider_->matches()[1].contents);
+    EXPECT_EQ(base::UTF8ToUTF16("map"), provider_->matches()[2].contents);
+  }
+
+  // Test "always-serve" mode.
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kOnDeviceHeadProvider,
+        {{"IncognitoServeMode", "always-serve"}});
+
+    provider_->Start(input, false);
+    if (!provider_->done())
+      task_environment_.RunUntilIdle();
+
+    EXPECT_TRUE(provider_->done());
+    ASSERT_EQ(3U, provider_->matches().size());
+    EXPECT_EQ(base::UTF8ToUTF16("maps"), provider_->matches()[0].contents);
+    EXPECT_EQ(base::UTF8ToUTF16("mail"), provider_->matches()[1].contents);
+    EXPECT_EQ(base::UTF8ToUTF16("map"), provider_->matches()[2].contents);
+  }
 }
 
 TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) {
@@ -124,7 +166,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) {
   input.set_want_asynchronous_matches(true);
   input.set_from_omnibox_focus(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
   EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
 
   provider_->Start(input, false);
@@ -141,7 +183,7 @@ TEST_F(OnDeviceHeadProviderTest, NoMatches) {
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
   EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
 
   provider_->Start(input, false);
@@ -158,7 +200,7 @@ TEST_F(OnDeviceHeadProviderTest, HasMatches) {
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
   EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
 
   provider_->Start(input, false);