Commit 7c554ad1 authored by manukh's avatar manukh Committed by Chromium LUCI CQ

[omnibox] [doc|drive] Set param defaults to launch values.

This does not enable the drive feature by default as it's currently
limited to enterprise users.

Sets:
- DocumentUseClientScore to true
- DocumentProviderMaxQueryLength to 200
- DebounceDocumentProviderDelayMs to 300

Change-Id: Idf85fcf6b25a844b196fa5ae82d5ac7617e6dad9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575378
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: default avatarJustin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834350}
parent 0ef9d928
...@@ -527,7 +527,7 @@ DocumentProvider::DocumentProvider(AutocompleteProviderClient* client, ...@@ -527,7 +527,7 @@ DocumentProvider::DocumentProvider(AutocompleteProviderClient* client,
static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt( static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt(
omnibox::kDocumentProvider, omnibox::kDocumentProvider,
"DocumentProviderMaxQueryLength", "DocumentProviderMaxQueryLength",
-1))), 200))),
min_query_show_length_( min_query_show_length_(
static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt( static_cast<size_t>(base::GetFieldTrialParamByFeatureAsInt(
omnibox::kDocumentProvider, omnibox::kDocumentProvider,
...@@ -561,7 +561,7 @@ DocumentProvider::DocumentProvider(AutocompleteProviderClient* client, ...@@ -561,7 +561,7 @@ DocumentProvider::DocumentProvider(AutocompleteProviderClient* client,
"DebounceDocumentProviderFromLastRun", true); "DebounceDocumentProviderFromLastRun", true);
int delay_ms = base::GetFieldTrialParamByFeatureAsInt( int delay_ms = base::GetFieldTrialParamByFeatureAsInt(
omnibox::kDebounceDocumentProvider, "DebounceDocumentProviderDelayMs", omnibox::kDebounceDocumentProvider, "DebounceDocumentProviderDelayMs",
100); 300);
debouncer_ = std::make_unique<AutocompleteProviderDebouncer>(from_last_run, debouncer_ = std::make_unique<AutocompleteProviderDebouncer>(from_last_run,
delay_ms); delay_ms);
} else } else
...@@ -705,7 +705,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults( ...@@ -705,7 +705,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
// two scores will be used. // two scores will be used.
// If both are false, the server score will be used. // If both are false, the server score will be used.
bool use_client_score = base::GetFieldTrialParamByFeatureAsBool( bool use_client_score = base::GetFieldTrialParamByFeatureAsBool(
omnibox::kDocumentProvider, "DocumentUseClientScore", false); omnibox::kDocumentProvider, "DocumentUseClientScore", true);
bool use_server_score = base::GetFieldTrialParamByFeatureAsBool( bool use_server_score = base::GetFieldTrialParamByFeatureAsBool(
omnibox::kDocumentProvider, "DocumentUseServerScore", true); omnibox::kDocumentProvider, "DocumentUseServerScore", true);
...@@ -799,7 +799,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults( ...@@ -799,7 +799,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
// Decrement scores if necessary to ensure suggestion order is preserved. // Decrement scores if necessary to ensure suggestion order is preserved.
// Don't decrement client scores which don't necessarily rank suggestions // Don't decrement client scores which don't necessarily rank suggestions
// the same as the server. // the same order as the server.
if (!use_client_score && score >= previous_score) if (!use_client_score && score >= previous_score)
score = std::max(previous_score - 1, 0); score = std::max(previous_score - 1, 0);
previous_score = score; previous_score = score;
......
...@@ -305,13 +305,13 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) { ...@@ -305,13 +305,13 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) {
R"({ R"({
"results": [ "results": [
{ {
"title": "Document 1", "title": "Document 1 longer title",
"url": "https://documentprovider.tld/doc?id=1", "url": "https://documentprovider.tld/doc?id=1",
"score": 1234, "score": 1234,
"originalUrl": "%s" "originalUrl": "%s"
}, },
{ {
"title": "Document 2", "title": "Document 2 longer title",
"url": "https://documentprovider.tld/doc?id=2" "url": "https://documentprovider.tld/doc?id=2"
} }
] ]
...@@ -323,17 +323,21 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) { ...@@ -323,17 +323,21 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) {
ASSERT_TRUE(response); ASSERT_TRUE(response);
ASSERT_TRUE(response->is_dict()); ASSERT_TRUE(response->is_dict());
provider_->input_.UpdateText(base::UTF8ToUTF16("input"), 0, {}); // Docs scores are the min of the server and client scores. To avoid client
// scores coming into play in this test, set the input to match the title
// similarly enough that the client score will surpass the server score.
provider_->input_.UpdateText(base::UTF8ToUTF16("document longer title"), 0,
{});
ACMatches matches = provider_->ParseDocumentSearchResults(*response); ACMatches matches = provider_->ParseDocumentSearchResults(*response);
EXPECT_EQ(matches.size(), 2u); EXPECT_EQ(matches.size(), 2u);
EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1")); EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("Document 1 longer title"));
EXPECT_EQ(matches[0].destination_url, EXPECT_EQ(matches[0].destination_url,
GURL("https://documentprovider.tld/doc?id=1")); GURL("https://documentprovider.tld/doc?id=1"));
EXPECT_EQ(matches[0].relevance, 1234); // Server-specified. EXPECT_EQ(matches[0].relevance, 1234); // Server-specified.
EXPECT_EQ(matches[0].stripped_destination_url, GURL(SAMPLE_STRIPPED_URL)); EXPECT_EQ(matches[0].stripped_destination_url, GURL(SAMPLE_STRIPPED_URL));
EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("Document 2")); EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("Document 2 longer title"));
EXPECT_EQ(matches[1].destination_url, EXPECT_EQ(matches[1].destination_url,
GURL("https://documentprovider.tld/doc?id=2")); GURL("https://documentprovider.tld/doc?id=2"));
EXPECT_EQ(matches[1].relevance, 0); EXPECT_EQ(matches[1].relevance, 0);
...@@ -547,6 +551,11 @@ TEST_F(DocumentProviderTest, MatchDescriptionString) { ...@@ -547,6 +551,11 @@ TEST_F(DocumentProviderTest, MatchDescriptionString) {
} }
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) { TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) {
// Tie breaking is disabled when client scoring is enabled.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
omnibox::kDocumentProvider, {{"DocumentUseClientScore", "false"}});
const std::string kGoodJSONResponseWithTies = base::StringPrintf( const std::string kGoodJSONResponseWithTies = base::StringPrintf(
R"({ R"({
"results": [ "results": [
...@@ -603,6 +612,11 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) { ...@@ -603,6 +612,11 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) {
} }
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) { TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
// Tie breaking is disabled when client scoring is enabled.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
omnibox::kDocumentProvider, {{"DocumentUseClientScore", "false"}});
const std::string kGoodJSONResponseWithTies = base::StringPrintf( const std::string kGoodJSONResponseWithTies = base::StringPrintf(
R"({ R"({
"results": [ "results": [
...@@ -661,6 +675,11 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) { ...@@ -661,6 +675,11 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
} }
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) { TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) {
// Tie breaking is disabled when client scoring is enabled.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeatureWithParameters(
omnibox::kDocumentProvider, {{"DocumentUseClientScore", "false"}});
const std::string kGoodJSONResponseWithTies = base::StringPrintf( const std::string kGoodJSONResponseWithTies = base::StringPrintf(
R"({ R"({
"results": [ "results": [
...@@ -978,7 +997,7 @@ TEST_F(DocumentProviderTest, Caching) { ...@@ -978,7 +997,7 @@ TEST_F(DocumentProviderTest, Caching) {
for (auto doc_id : doc_ids) for (auto doc_id : doc_ids)
results += base::StringPrintf( results += base::StringPrintf(
R"({ R"({
"title": "Document %s", "title": "Document %s longer title",
"score": 1150, "score": 1150,
"url": "https://drive.google.com/open?id=%s", "url": "https://drive.google.com/open?id=%s",
"originalUrl": "https://drive.google.com/open?id=%s", "originalUrl": "https://drive.google.com/open?id=%s",
...@@ -998,79 +1017,95 @@ TEST_F(DocumentProviderTest, Caching) { ...@@ -998,79 +1017,95 @@ TEST_F(DocumentProviderTest, Caching) {
auto matches = auto matches =
GetTestProviderMatches("input", MakeTestResponse({"0", "1", "2"})); GetTestProviderMatches("input", MakeTestResponse({"0", "1", "2"}));
EXPECT_EQ(matches.size(), size_t(3)); EXPECT_EQ(matches.size(), size_t(3));
EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0")); EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0 longer title"));
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 1")); EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 1 longer title"));
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 2")); EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 2 longer title"));
// Cache should remove duplicates. // Cache should remove duplicates.
matches = GetTestProviderMatches("input", MakeTestResponse({"1", "2", "3"})); matches = GetTestProviderMatches("input", MakeTestResponse({"1", "2", "3"}));
EXPECT_EQ(matches.size(), size_t(4)); EXPECT_EQ(matches.size(), size_t(4));
EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 1")); EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 1 longer title"));
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 2")); EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 2 longer title"));
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 3")); EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 3 longer title"));
EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0")); EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0 longer title"));
// Cache size (4) should not restrict number of matches from the current // Cache size (4) should not restrict number of matches from the current
// response. // response.
matches = GetTestProviderMatches("input", MakeTestResponse({"3", "4", "5"})); matches = GetTestProviderMatches("input", MakeTestResponse({"3", "4", "5"}));
EXPECT_EQ(matches.size(), size_t(6)); EXPECT_EQ(matches.size(), size_t(6));
EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 3")); EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 3 longer title"));
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4")); EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4 longer title"));
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 5")); EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 5 longer title"));
EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 1")); EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 1 longer title"));
EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 2")); EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 2 longer title"));
EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 0")); EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 0 longer title"));
// Cache size (4) should restrict number of cached matches appended. // Cache size (4) should restrict number of cached matches appended.
matches = GetTestProviderMatches("input", MakeTestResponse({"0", "4", "6"})); matches = GetTestProviderMatches("input", MakeTestResponse({"0", "4", "6"}));
EXPECT_EQ(matches.size(), size_t(6)); EXPECT_EQ(matches.size(), size_t(6));
EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0")); EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 0 longer title"));
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4")); EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4 longer title"));
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 6")); EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 6 longer title"));
EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 3")); EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 3 longer title"));
EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 5")); EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 5 longer title"));
EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 1")); EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 1 longer title"));
// Cached results should update match |additional_info|, |relevance|, and // Cached results should update match |additional_info|, |relevance|, and
// |contents_class|. // |contents_class|.
matches = GetTestProviderMatches("docum", MakeTestResponse({"5", "4", "7"})); // Docs scores are the min of the server and client scores. To avoid client
// scores coming into play in this test, set the input to match the title
// similarly enough that the client score will surpass the server score.
matches = GetTestProviderMatches("docum longer title",
MakeTestResponse({"5", "4", "7"}));
EXPECT_EQ(matches.size(), size_t(6)); EXPECT_EQ(matches.size(), size_t(6));
EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 5")); EXPECT_EQ(matches[0].contents, base::UTF8ToUTF16("Document 5 longer title"));
EXPECT_EQ(matches[0].GetAdditionalInfo("from cache"), ""); EXPECT_EQ(matches[0].GetAdditionalInfo("from cache"), "");
EXPECT_EQ(matches[0].relevance, 1150); EXPECT_EQ(matches[0].relevance, 1150);
EXPECT_THAT(matches[0].contents_class, EXPECT_THAT(matches[0].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4")); ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
EXPECT_EQ(matches[1].contents, base::UTF8ToUTF16("Document 4 longer title"));
EXPECT_EQ(matches[1].GetAdditionalInfo("from cache"), ""); EXPECT_EQ(matches[1].GetAdditionalInfo("from cache"), "");
EXPECT_EQ(matches[1].relevance, 1149); EXPECT_EQ(matches[1].relevance, 1150);
EXPECT_THAT(matches[1].contents_class, EXPECT_THAT(matches[1].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 7")); ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
EXPECT_EQ(matches[2].contents, base::UTF8ToUTF16("Document 7 longer title"));
EXPECT_EQ(matches[2].GetAdditionalInfo("from cache"), ""); EXPECT_EQ(matches[2].GetAdditionalInfo("from cache"), "");
EXPECT_EQ(matches[2].relevance, 1148); EXPECT_EQ(matches[2].relevance, 1150);
EXPECT_THAT(matches[2].contents_class, EXPECT_THAT(matches[2].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0")); ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
EXPECT_EQ(matches[3].contents, base::UTF8ToUTF16("Document 0 longer title"));
EXPECT_EQ(matches[3].GetAdditionalInfo("from cache"), "true"); EXPECT_EQ(matches[3].GetAdditionalInfo("from cache"), "true");
EXPECT_EQ(matches[3].relevance, 0); EXPECT_EQ(matches[3].relevance, 0);
EXPECT_THAT(matches[3].contents_class, EXPECT_THAT(matches[3].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 6")); ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
EXPECT_EQ(matches[4].contents, base::UTF8ToUTF16("Document 6 longer title"));
EXPECT_EQ(matches[4].GetAdditionalInfo("from cache"), "true"); EXPECT_EQ(matches[4].GetAdditionalInfo("from cache"), "true");
EXPECT_EQ(matches[4].relevance, 0); EXPECT_EQ(matches[4].relevance, 0);
EXPECT_THAT(matches[4].contents_class, EXPECT_THAT(matches[4].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 3")); ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
EXPECT_EQ(matches[5].contents, base::UTF8ToUTF16("Document 3 longer title"));
EXPECT_EQ(matches[5].GetAdditionalInfo("from cache"), "true"); EXPECT_EQ(matches[5].GetAdditionalInfo("from cache"), "true");
EXPECT_EQ(matches[5].relevance, 0); EXPECT_EQ(matches[5].relevance, 0);
EXPECT_THAT(matches[5].contents_class, EXPECT_THAT(matches[5].contents_class,
testing::ElementsAre(ACMatchClassification{0, 2}, testing::ElementsAre(
ACMatchClassification{5, 0})); ACMatchClassification{0, 2}, ACMatchClassification{5, 0},
ACMatchClassification{11, 2}, ACMatchClassification{17, 0},
ACMatchClassification{18, 2}));
} }
TEST_F(DocumentProviderTest, MinQueryLength) { TEST_F(DocumentProviderTest, MinQueryLength) {
......
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