[omnibox] re-land https://crrev.com/c/1881978 which was reverted due to

the flaky unittest.

Here we simplify unittests, especially "TestIncognito" by only checking
return value of OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed.
This unittest in the previous change was too heavy that it fired
mulitple async calls and might eventually caused data race in
ScopedFeatureList on some platforms: http://screenshot/8x1yJO66CjU.

Non test cc files are the same as previous CL.

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

components/omnibox/browser/omnibox_field_trial.cc

@@ -680,11 +680,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())
-    return false;
+  // 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"
 
@@ -63,6 +65,10 @@ class OnDeviceHeadProviderTest : public testing::Test,
     }
   }
 
+  bool IsOnDeviceHeadProviderAllowed(const AutocompleteInput& input) {
+    return provider_->IsOnDeviceHeadProviderAllowed(input);
+  }
+
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<FakeAutocompleteProviderClient> client_;
   scoped_refptr<OnDeviceHeadProvider> provider_;
@@ -75,8 +81,11 @@ TEST_F(OnDeviceHeadProviderTest, ServingInstanceNotCreated) {
   input.set_want_asynchronous_matches(true);
   ResetServingInstance();
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
-  EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
+  EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
+      .WillRepeatedly(Return(true));
+
+  ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
 
   provider_->Start(input, false);
   if (!provider_->done())
@@ -92,29 +101,39 @@ TEST_F(OnDeviceHeadProviderTest, RejectSynchronousRequest) {
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(false);
 
-  provider_->Start(input, false);
-  if (!provider_->done())
-    task_environment_.RunUntilIdle();
-
-  EXPECT_TRUE(provider_->matches().empty());
-  EXPECT_TRUE(provider_->done());
+  ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input));
 }
 
-TEST_F(OnDeviceHeadProviderTest, RejectIncognito) {
+TEST_F(OnDeviceHeadProviderTest, TestIfIncognitoIsAllowed) {
   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));
 
-  provider_->Start(input, false);
-  if (!provider_->done())
-    task_environment_.RunUntilIdle();
+  // By default incognito request will be rejected.
+  ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input));
 
-  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"}});
+    ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
+  }
+
+  // Test "always-serve" mode.
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeatureWithParameters(
+        omnibox::kOnDeviceHeadProvider,
+        {{"IncognitoServeMode", "always-serve"}});
+    ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
+  }
 }
 
 TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) {
@@ -124,15 +143,10 @@ 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);
-  if (!provider_->done())
-    task_environment_.RunUntilIdle();
-
-  EXPECT_TRUE(provider_->matches().empty());
-  EXPECT_TRUE(provider_->done());
+  ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input));
 }
 
 TEST_F(OnDeviceHeadProviderTest, NoMatches) {
@@ -141,8 +155,11 @@ TEST_F(OnDeviceHeadProviderTest, NoMatches) {
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
-  EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
+  EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
+      .WillRepeatedly(Return(true));
+
+  ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
 
   provider_->Start(input, false);
   if (!provider_->done())
@@ -158,8 +175,11 @@ TEST_F(OnDeviceHeadProviderTest, HasMatches) {
                           TestSchemeClassifier());
   input.set_want_asynchronous_matches(true);
 
-  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
-  EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
+  EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
+  EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
+      .WillRepeatedly(Return(true));
+
+  ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
 
   provider_->Start(input, false);
   if (!provider_->done())
@@ -186,6 +206,9 @@ TEST_F(OnDeviceHeadProviderTest, CancelInProgressRequest) {
   EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
       .WillRepeatedly(Return(true));
 
+  ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input1));
+  ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input2));
+
   provider_->Start(input1, false);
   EXPECT_FALSE(provider_->done());
   provider_->Start(input2, false);