Revert "Refactor SearchProviderTest.SendsWarmUpRequestOnFocus to Deflake It"

This reverts commit 6111392d.

Refactoring did do something, but the issue remained.  The core problem
I think is that the parent class fields are initialized first.  This
means the parent class's fields like content::TestBrowserThreadBundle
get created, which I think will end up looking at field trial state.
Adding a base::test::ScopedFeatureList after that happens is too late;
the initial field trial state has already been inspected.

I could create a separate class that lists the base::test::ScopedFeatureList
first, and then all the other variables such as TestBrowserThreadBundle
later.  That might work.  Or not; I think there are other reasons the
TestBrowserThreadBundle wants to be listed first in the fields of a testing
class.

It's not worth the trouble to investigate.  I'm happy to have coverage
of the single MAYBE_ test on regular bot.  I don't think we also need
coverage of it on the ASAN (THREAD_SANITIZER) bots as well.

Bug: 891959
Change-Id: Iae816dde484982060121c3e667b779d70da00e65
Reviewed-on: https://chromium-review.googlesource.com/c/1334269Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607776}

chrome/browser/autocomplete/search_provider_unittest.cc

@@ -3614,31 +3614,6 @@ TEST_F(SearchProviderTest, DoesNotProvideOnFocus) {
   EXPECT_TRUE(provider_->matches().empty());
 }
 
-// SearchProviderWarmUpTest --------------------------------------------------
-//
-// Like SearchProviderTest.  The only addition is that it's a
-// TestWithParam<bool>, where the boolean parameter represents whether the
-// omnibox::kSearchProviderWarmUpOnFocus feature flag should be enabled.
-class SearchProviderWarmUpTest : public SearchProviderTest,
-                                 public testing::WithParamInterface<bool> {
- public:
-  SearchProviderWarmUpTest() {}
-  void SetUp() override;
-
- protected:
-  base::test::ScopedFeatureList feature_list_;
-
-  DISALLOW_COPY_AND_ASSIGN(SearchProviderWarmUpTest);
-};
-
-void SearchProviderWarmUpTest::SetUp() {
-  if (GetParam())
-    feature_list_.InitAndEnableFeature(omnibox::kSearchProviderWarmUpOnFocus);
-  else
-    feature_list_.InitAndDisableFeature(omnibox::kSearchProviderWarmUpOnFocus);
-  SearchProviderTest::SetUp();
-}
-
 #if defined(THREAD_SANITIZER)
 // SearchProviderTest.SendsWarmUpRequestOnFocus is flaky on Linux TSan Tests
 // crbug.com/891959.
@@ -3646,22 +3621,30 @@ void SearchProviderWarmUpTest::SetUp() {
 #else
 #define MAYBE_SendsWarmUpRequestOnFocus SendsWarmUpRequestOnFocus
 #endif  // defined(THREAD_SANITIZER)
-TEST_P(SearchProviderWarmUpTest, MAYBE_SendsWarmUpRequestOnFocus) {
+TEST_F(SearchProviderTest, MAYBE_SendsWarmUpRequestOnFocus) {
   AutocompleteInput input(base::ASCIIToUTF16("f"),
                           metrics::OmniboxEventProto::OTHER,
                           ChromeAutocompleteSchemeClassifier(&profile_));
   input.set_prefer_keyword(true);
   input.set_from_omnibox_focus(true);
 
-  if (!GetParam()) {  // The warm-up feature ought to be disabled.
-    // The provider immediately terminates with no matches.
+  {
+    // First, verify that without the warm-up feature enabled, the provider
+    // immediately terminates with no matches.
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndDisableFeature(omnibox::kSearchProviderWarmUpOnFocus);
     provider_->Start(input, false);
     // RunUntilIdle so that SearchProvider has a chance to create the
     // URLFetchers (if it wants to, which it shouldn't in this case).
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(provider_->done());
     EXPECT_TRUE(provider_->matches().empty());
-  } else {  // The warm-up feature ought to be enabled.
+  }
+
+  {
+    // Then, check the behavior with the warm-up feature enabled.
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(omnibox::kSearchProviderWarmUpOnFocus);
     provider_->Start(input, false);
     // RunUntilIdle so that SearchProvider create the URLFetcher.
     base::RunLoop().RunUntilIdle();
@@ -3679,7 +3662,3 @@ TEST_P(SearchProviderWarmUpTest, MAYBE_SendsWarmUpRequestOnFocus) {
     EXPECT_TRUE(provider_->matches().empty());
   }
 }
-
-INSTANTIATE_TEST_CASE_P(SearchProviderTest,
-                        SearchProviderWarmUpTest,
-                        testing::Values(false, true));

components/omnibox/browser/search_provider.h

@@ -95,13 +95,13 @@ class SearchProvider : public BaseSearchProvider,
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, AnswersCache);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, RemoveExtraAnswers);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoesNotProvideOnFocus);
+  FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, SendsWarmUpRequestOnFocus);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
                            DontTrimHttpSchemeIfInputHasScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest,
                            DontTrimHttpsSchemeIfInputHasScheme);
   FRIEND_TEST_ALL_PREFIXES(SearchProviderTest, DoTrimHttpsScheme);
-  FRIEND_TEST_ALL_PREFIXES(SearchProviderWarmUpTest, SendsWarmUpRequestOnFocus);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, ClearPrefetchedResults);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedPrefetchTest, SetPrefetchQuery);