Skip to content

GitLab

T
tangled
Commit 7dd0d17a authored by manukh's avatar manukh Committed by Commit Bot
Browse files
Options

[omnibox] [drive] Remove deprecated drive backoff mechanism. 

At one point, the document provider expected responses with http status
codes of 200 with a specific error code in the response body. This was
deprecated in favor of responses with 400 & 499 http status codes. For a
while, the provider handled both. Now that the backend exclusively
employs the new responses, it's safe to remove the original 200 backoff
handling.

Change-Id: Ie79fedaa6e0930db3f32aefedd3ccde699f984a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2416561Reviewed-by: default avatarJustin Donnelly <jdonnelly@chromium.org>
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808465}
parent 28ca7967
Hide whitespace changes
Inline Side-by-side
Showing with 16 additions and 103 deletions
chrome/browser/about_flags.cc
View file @ 7dd0d17a
......@@ -3815,9 +3815,10 @@ const FeatureEntry kFeatureEntries[] = {
{"omnibox-drive-suggestions",
flag_descriptions::kOmniboxDriveSuggestionsName,
flag_descriptions::kOmniboxDriveSuggestionsDescriptions, kOsDesktop,
FEATURE_WITH_PARAMS_VALUE_TYPE(omnibox::kDocumentProvider,
kOmniboxDocumentProviderVariations,
"OmniboxBundledExperimentV1")},
FEATURE_WITH_PARAMS_VALUE_TYPE(
omnibox::kDocumentProvider,
kOmniboxDocumentProviderVariations,
"OmniboxDocumentProviderNonDogfoodExperiments")},
{"omnibox-autocomplete-titles",
flag_descriptions::kOmniboxAutocompleteTitlesName,
flag_descriptions::kOmniboxAutocompleteTitlesDescription, kOsDesktop,
......
components/omnibox/browser/document_provider.cc
View file @ 7dd0d17a
......@@ -101,40 +101,6 @@ AutocompleteMatch::DocumentType GetIconForMIMEType(
: AutocompleteMatch::DocumentType::DRIVE_OTHER;
}
const char kErrorMessageAdminDisabled[] =
"Not eligible to query due to admin disabled Chrome search settings.";
const char kErrorMessageRetryLater[] = "Not eligible to query, see retry info.";
// TODO(manukh): Remove ResponseContainsBackoffSignal once the check using http
// status code in |OnURLLoadComplete| rolls out and the backend returns to
// sending 4xx backoff responses as opposed to 2xx; or, if the backend is never
// adjusted to send 2xx responses, once that check rolls out.
bool ResponseContainsBackoffSignal(const base::DictionaryValue* root_dict) {
const base::DictionaryValue* error_info;
if (!root_dict->GetDictionary("error", &error_info)) {
return false;
}
int code;
std::string status;
std::string message;
if (!error_info->GetInteger("code", &code) ||
!error_info->GetString("status", &status) ||
!error_info->GetString("message", &message)) {
return false;
}
// 403/PERMISSION_DENIED: Account is currently ineligible to receive results.
if (code == 403 && status == "PERMISSION_DENIED" &&
message == kErrorMessageAdminDisabled) {
return true;
}
// 503/UNAVAILABLE: Uninteresting set of results, or another server request to
// backoff.
return code == 503 && status == "UNAVAILABLE" &&
message == kErrorMessageRetryLater;
}
struct FieldMatches {
double weight;
String16Vector words;
......@@ -732,15 +698,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
return matches;
}
// The server may ask the client to back off, in which case we back off for
// the session.
// TODO(skare): Respect retryDelay if provided, ideally by calling via gRPC.
if (ResponseContainsBackoffSignal(root_dict)) {
backoff_for_session_ = true;
return matches;
}
// Otherwise parse the results.
// Parse the results.
if (!root_dict->GetList("results", &results_list)) {
return matches;
}
......@@ -794,7 +752,7 @@ ACMatches DocumentProvider::ParseDocumentSearchResults(
WithinBounds(input_.text().length(), min_query_show_length_,
max_query_show_length_);
// In order to compare small slices of input length while excluding noise from
// the larger group of all input lenghts, |min_query_log_length_| and
// the larger group of all input lengths, |min_query_log_length_| and
// |max_query_log_length_| specify the queries that will log
// field_trial_triggered. E.g., if |min_query_log_length_| is 50 and
// |max_query_log_length_| is -1, only inputs of length 50 or greater which
......
components/omnibox/browser/document_provider.h
View file @ 7dd0d17a
......@@ -101,9 +101,7 @@ class DocumentProvider : public AutocompleteProvider {
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
ParseDocumentSearchResultsBreakTiesZeroLimit);
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
ParseDocumentSearchResultsWithBackoff);
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest,
ParseDocumentSearchResultsWithIneligibleFlag);
ParseDocumentSearchResultsWithBadResponse);
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, GenerateLastModifiedString);
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Scoring);
FRIEND_TEST_ALL_PREFIXES(DocumentProviderTest, Caching);
......
components/omnibox/browser/document_provider_unittest.cc
View file @ 7dd0d17a
......@@ -271,7 +271,6 @@ TEST_F(DocumentProviderTest, CheckFeaturePrerequisiteServerBackoff) {
provider_->backoff_for_session_ = true;
EXPECT_FALSE(
provider_->IsDocumentProviderAllowed(client_.get(), AutocompleteInput()));
provider_->backoff_for_session_ = false;
}
TEST_F(DocumentProviderTest, IsInputLikelyURL) {
......@@ -340,7 +339,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResults) {
EXPECT_EQ(matches[1].relevance, 0);
EXPECT_TRUE(matches[1].stripped_destination_url.is_empty());
ASSERT_FALSE(provider_->backoff_for_session_);
EXPECT_FALSE(provider_->backoff_for_session_);
}
TEST_F(DocumentProviderTest, ProductDescriptionStringsAndAccessibleLabels) {
......@@ -600,7 +599,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTies) {
EXPECT_EQ(matches[2].relevance, 1232); // Tie demoted, twice.
EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
ASSERT_FALSE(provider_->backoff_for_session_);
EXPECT_FALSE(provider_->backoff_for_session_);
}
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
......@@ -658,7 +657,7 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesCascade) {
EXPECT_EQ(matches[2].relevance, 1232);
EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
ASSERT_FALSE(provider_->backoff_for_session_);
EXPECT_FALSE(provider_->backoff_for_session_);
}
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) {
......@@ -715,45 +714,10 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsBreakTiesZeroLimit) {
EXPECT_EQ(matches[2].relevance, 0);
EXPECT_TRUE(matches[2].stripped_destination_url.is_empty());
ASSERT_FALSE(provider_->backoff_for_session_);
}
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithBackoff) {
// Response where the server wishes to trigger backoff.
const char kBackoffJSONResponse[] = R"({
"error": {
"code": 503,
"message": "Not eligible to query, see retry info.",
"status": "UNAVAILABLE",
"details": [
{
"@type": "type.googleapis.com/google.rpc.RetryInfo",
"retryDelay": "100000s"
},
]
}
})";
ASSERT_FALSE(provider_->backoff_for_session_);
base::Optional<base::Value> backoff_response = base::JSONReader::Read(
kBackoffJSONResponse, base::JSON_ALLOW_TRAILING_COMMAS);
ASSERT_TRUE(backoff_response);
ASSERT_TRUE(backoff_response->is_dict());
ACMatches matches = provider_->ParseDocumentSearchResults(*backoff_response);
ASSERT_TRUE(provider_->backoff_for_session_);
EXPECT_FALSE(provider_->backoff_for_session_);
}
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithIneligibleFlag) {
// Response where the server wishes to trigger backoff.
const char kIneligibleJSONResponse[] = R"({
"error": {
"code": 403,
"message": "Not eligible to query due to admin disabled Chrome search settings.",
"status": "PERMISSION_DENIED",
}
})";
TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithBadResponse) {
// Same as above, but the message doesn't match. We should accept this
// response, but it isn't expected to trigger backoff.
const char kMismatchedMessageJSON[] = R"({
......@@ -767,22 +731,14 @@ TEST_F(DocumentProviderTest, ParseDocumentSearchResultsWithIneligibleFlag) {
ACMatches matches;
ASSERT_FALSE(provider_->backoff_for_session_);
// First, parse an invalid response - shouldn't prohibit future requests
// from working but also shouldn't trigger backoff.
base::Optional<base::Value> bad_response = base::JSONReader::Read(
kMismatchedMessageJSON, base::JSON_ALLOW_TRAILING_COMMAS);
ASSERT_TRUE(bad_response);
ASSERT_TRUE(bad_response->is_dict());
matches = provider_->ParseDocumentSearchResults(*bad_response);
ASSERT_FALSE(provider_->backoff_for_session_);
// Now parse a response that does trigger backoff.
base::Optional<base::Value> backoff_response = base::JSONReader::Read(
kIneligibleJSONResponse, base::JSON_ALLOW_TRAILING_COMMAS);
ASSERT_TRUE(backoff_response);
ASSERT_TRUE(backoff_response->is_dict());
matches = provider_->ParseDocumentSearchResults(*backoff_response);
ASSERT_TRUE(provider_->backoff_for_session_);
EXPECT_EQ(matches.size(), 0u);
// Shouldn't prohibit future requests or trigger backoff.
EXPECT_FALSE(provider_->backoff_for_session_);
}
// This test is affected by an iOS 10 simulator bug: https://crbug.com/782033
......@@ -803,7 +759,7 @@ TEST_F(DocumentProviderTest, GenerateLastModifiedString) {
base::Time modified_this_year = local_now + base::TimeDelta::FromDays(-8);
base::Time modified_last_year = local_now + base::TimeDelta::FromDays(-365);
// GenerateLastModifiedString should accept any parseable timestamp, but use
// GenerateLastModifiedString should accept any parsable timestamp, but use
// ISO8601 UTC timestamp strings since the service returns them in practice.
EXPECT_EQ(DocumentProvider::GenerateLastModifiedString(
base::TimeToISO8601(modified_today), local_now),
......
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