Commit 72de1a08 authored by Ce Chen's avatar Ce Chen Committed by Commit Bot

[omnibox] get rid of test::ScopedFeatureList from on device head

provider unittest.

Its function base::FieldTrialList::BackupInstanceForTesting()
causes flakiness in chrome linux tsan test, e.g.
https://screenshot.googleplex.com/NF0rm8HZPhn
https://screenshot.googleplex.com/NvL2qn78tLk

I've put https://crrev.com/c/1888691 in patchset1 as a reference for
code review (in case the previous CL is reverted).

Change-Id: Ib34273087bc142df3efd122d13b08879f4b81a23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1895869Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Commit-Queue: Ce Chen <cch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712270}
parent 00305d2e
...@@ -98,7 +98,8 @@ OnDeviceHeadProvider::~OnDeviceHeadProvider() { ...@@ -98,7 +98,8 @@ OnDeviceHeadProvider::~OnDeviceHeadProvider() {
} }
bool OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed( bool OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed(
const AutocompleteInput& input) { const AutocompleteInput& input,
const std::string& incognito_serve_mode) {
// Only accept asynchronous request. // Only accept asynchronous request.
if (!input.want_asynchronous_matches() || if (!input.want_asynchronous_matches() ||
input.type() == metrics::OmniboxInputType::EMPTY) input.type() == metrics::OmniboxInputType::EMPTY)
...@@ -114,12 +115,10 @@ bool OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed( ...@@ -114,12 +115,10 @@ bool OnDeviceHeadProvider::IsOnDeviceHeadProviderAllowed(
// request; this is the default behavior; // request; this is the default behavior;
// 2. incognito-only: only serve incognito request; // 2. incognito-only: only serve incognito request;
// 3. always-serve: always serve regardless of incognito. // 3. always-serve: always serve regardless of incognito.
const std::string mode = base::GetFieldTrialParamValueByFeature( if (incognito_serve_mode != "always-serve") {
omnibox::kOnDeviceHeadProvider, "IncognitoServeMode"); if (client()->IsOffTheRecord() && incognito_serve_mode != "incognito-only")
if (mode != "always-serve") {
if (client()->IsOffTheRecord() && mode != "incognito-only")
return false; return false;
if (!client()->IsOffTheRecord() && mode == "incognito-only") if (!client()->IsOffTheRecord() && incognito_serve_mode == "incognito-only")
return false; return false;
} }
...@@ -138,7 +137,9 @@ void OnDeviceHeadProvider::Start(const AutocompleteInput& input, ...@@ -138,7 +137,9 @@ void OnDeviceHeadProvider::Start(const AutocompleteInput& input,
// Cancel any in-progress request. // Cancel any in-progress request.
Stop(!minimal_changes, false); Stop(!minimal_changes, false);
if (!IsOnDeviceHeadProviderAllowed(input)) { const std::string mode = base::GetFieldTrialParamValueByFeature(
omnibox::kOnDeviceHeadProvider, "IncognitoServeMode");
if (!IsOnDeviceHeadProviderAllowed(input, mode)) {
matches_.clear(); matches_.clear();
return; return;
} }
......
...@@ -45,7 +45,8 @@ class OnDeviceHeadProvider : public AutocompleteProvider { ...@@ -45,7 +45,8 @@ class OnDeviceHeadProvider : public AutocompleteProvider {
AutocompleteProviderListener* listener); AutocompleteProviderListener* listener);
~OnDeviceHeadProvider() override; ~OnDeviceHeadProvider() override;
bool IsOnDeviceHeadProviderAllowed(const AutocompleteInput& input); bool IsOnDeviceHeadProviderAllowed(const AutocompleteInput& input,
const std::string& incognito_serve_mode);
// Helper functions used for asynchronous search to the on device head model. // Helper functions used for asynchronous search to the on device head model.
// The Autocomplete input and output from the model will be passed from // The Autocomplete input and output from the model will be passed from
......
...@@ -8,7 +8,6 @@ ...@@ -8,7 +8,6 @@
#include "base/path_service.h" #include "base/path_service.h"
#include "base/strings/string_util.h" #include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h" #include "base/test/task_environment.h"
#include "build/build_config.h" #include "build/build_config.h"
#include "components/omnibox/browser/autocomplete_input.h" #include "components/omnibox/browser/autocomplete_input.h"
...@@ -16,7 +15,6 @@ ...@@ -16,7 +15,6 @@
#include "components/omnibox/browser/fake_autocomplete_provider_client.h" #include "components/omnibox/browser/fake_autocomplete_provider_client.h"
#include "components/omnibox/browser/on_device_head_serving.h" #include "components/omnibox/browser/on_device_head_serving.h"
#include "components/omnibox/browser/test_scheme_classifier.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/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
...@@ -65,8 +63,10 @@ class OnDeviceHeadProviderTest : public testing::Test, ...@@ -65,8 +63,10 @@ class OnDeviceHeadProviderTest : public testing::Test,
} }
} }
bool IsOnDeviceHeadProviderAllowed(const AutocompleteInput& input) { bool IsOnDeviceHeadProviderAllowed(const AutocompleteInput& input,
return provider_->IsOnDeviceHeadProviderAllowed(input); const std::string& incognito_serve_mode) {
return provider_->IsOnDeviceHeadProviderAllowed(input,
incognito_serve_mode);
} }
base::test::TaskEnvironment task_environment_; base::test::TaskEnvironment task_environment_;
...@@ -85,7 +85,7 @@ TEST_F(OnDeviceHeadProviderTest, ServingInstanceNotCreated) { ...@@ -85,7 +85,7 @@ TEST_F(OnDeviceHeadProviderTest, ServingInstanceNotCreated) {
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()) EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input, ""));
provider_->Start(input, false); provider_->Start(input, false);
if (!provider_->done()) if (!provider_->done())
...@@ -101,7 +101,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectSynchronousRequest) { ...@@ -101,7 +101,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectSynchronousRequest) {
TestSchemeClassifier()); TestSchemeClassifier());
input.set_want_asynchronous_matches(false); input.set_want_asynchronous_matches(false);
ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input, ""));
} }
TEST_F(OnDeviceHeadProviderTest, TestIfIncognitoIsAllowed) { TEST_F(OnDeviceHeadProviderTest, TestIfIncognitoIsAllowed) {
...@@ -115,25 +115,13 @@ TEST_F(OnDeviceHeadProviderTest, TestIfIncognitoIsAllowed) { ...@@ -115,25 +115,13 @@ TEST_F(OnDeviceHeadProviderTest, TestIfIncognitoIsAllowed) {
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
// By default incognito request will be rejected. // By default incognito request will be rejected.
ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input, ""));
// Now enable for incognito only. // Now enable for incognito only.
{ ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input, "incognito-only"));
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
omnibox::kOnDeviceHeadProvider,
{{"IncognitoServeMode", "incognito-only"}});
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
}
// Test "always-serve" mode. // Test "always-serve" mode.
{ ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input, "always-serve"));
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
omnibox::kOnDeviceHeadProvider,
{{"IncognitoServeMode", "always-serve"}});
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input));
}
} }
TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) { TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) {
...@@ -146,7 +134,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) { ...@@ -146,7 +134,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectOnFocusRequest) {
EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false)); EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillRepeatedly(Return(false));
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true)); EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_FALSE(IsOnDeviceHeadProviderAllowed(input, ""));
} }
TEST_F(OnDeviceHeadProviderTest, NoMatches) { TEST_F(OnDeviceHeadProviderTest, NoMatches) {
...@@ -159,7 +147,7 @@ TEST_F(OnDeviceHeadProviderTest, NoMatches) { ...@@ -159,7 +147,7 @@ TEST_F(OnDeviceHeadProviderTest, NoMatches) {
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()) EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input, ""));
provider_->Start(input, false); provider_->Start(input, false);
if (!provider_->done()) if (!provider_->done())
...@@ -179,7 +167,7 @@ TEST_F(OnDeviceHeadProviderTest, HasMatches) { ...@@ -179,7 +167,7 @@ TEST_F(OnDeviceHeadProviderTest, HasMatches) {
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()) EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input)); ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input, ""));
provider_->Start(input, false); provider_->Start(input, false);
if (!provider_->done()) if (!provider_->done())
...@@ -206,8 +194,8 @@ TEST_F(OnDeviceHeadProviderTest, CancelInProgressRequest) { ...@@ -206,8 +194,8 @@ TEST_F(OnDeviceHeadProviderTest, CancelInProgressRequest) {
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()) EXPECT_CALL(*client_.get(), SearchSuggestEnabled())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input1)); ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input1, ""));
ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input2)); ASSERT_TRUE(IsOnDeviceHeadProviderAllowed(input2, ""));
provider_->Start(input1, false); provider_->Start(input1, false);
EXPECT_FALSE(provider_->done()); EXPECT_FALSE(provider_->done());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment