Skip to content

Commit

Permalink
[suggest-internals] Moves deletion requests to RemoteSuggestionsService
Browse files Browse the repository at this point in the history 
SuggestionDeletionHandler is a class, internal to BaseSearchProvider,
which is responsible for making deletion requests to the backend for
personalized suggestions. BaseSearchProvider creates a new instance
of this class for every deletion request and holds on to it until the
request has completed.

This encapsulation seems unnecessary and wasteful as the deletion
handler does not do much other than creating a loader, starting
download, and holding on to the loader. BaseSearchProvider could
simply hold on to the network::SimpleURLLoader reference itself.

This change moves the SuggestionDeletionHandler logic into the
RemoteSuggestionsService in order to centralize all Suggest requests
in one place.

One noteworthy change is that we now always pass InIncognito::kNo to
variations::AppendVariationsHeaderUnknownSignedIn(). This however OK
since deletion requests are expected in non-incognito state only as
remote suggestions are not served in incognito mode.

Bug: 1410570
Change-Id: Icc2a2ff750d6cfb82d0e7ef8250fab53c6c466cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4201067
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098187}
  • Loading branch information
Moe Ahmadi authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent 830bf03 commit e53dc84
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 125 deletions.
26 changes: 20 additions & 6 deletions chrome/browser/autocomplete/search_provider_unittest.cc
View file
Expand Up @@ -22,6 +22,7 @@
#include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
#include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
#include "chrome/browser/autocomplete/remote_suggestions_service_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "chrome/browser/signin/chrome_signin_client_factory.h"
Expand All @@ -39,6 +40,7 @@
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/history_url_provider.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/remote_suggestions_service.h"
#include "components/omnibox/browser/search_provider.h"
#include "components/omnibox/browser/suggestion_answer.h"
#include "components/omnibox/common/omnibox_features.h"
Expand Down Expand Up @@ -75,7 +77,6 @@ ACMatches::const_iterator FindDefaultMatch(const ACMatches& matches) {
return it;
}

class SuggestionDeletionHandler;
class SearchProviderForTest : public SearchProvider {
public:
SearchProviderForTest(AutocompleteProviderClient* client,
Expand Down Expand Up @@ -136,6 +137,14 @@ class TestAutocompleteProviderClient : public ChromeAutocompleteProviderClient {
scoped_refptr<network::SharedURLLoaderFactory> shared_factory_;
};

std::unique_ptr<KeyedService> BuildRemoteSuggestionsServiceWithURLLoader(
network::TestURLLoaderFactory* test_url_loader_factory,
content::BrowserContext* context) {
return std::make_unique<RemoteSuggestionsService>(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
test_url_loader_factory));
}

} // namespace

// SearchProviderFeatureTestComponent -----------------------------------------
Expand Down Expand Up @@ -216,7 +225,8 @@ class BaseSearchProviderTest : public testing::Test,
explicit BaseSearchProviderTest(const bool command_line_overrides = false)
: feature_test_component_(command_line_overrides) {
// We need the history service, the template url model, and the signin
// client initialized with a TestURLLoaderFactory.
// client and the remote suggestions service initialized with a
// TestURLLoaderFactory.
TestingProfile::Builder profile_builder;
profile_builder.AddTestingFactory(
HistoryServiceFactory::GetInstance(),
Expand All @@ -228,6 +238,10 @@ class BaseSearchProviderTest : public testing::Test,
ChromeSigninClientFactory::GetInstance(),
base::BindRepeating(&BuildChromeSigninClientWithURLLoader,
&test_url_loader_factory_));
profile_builder.AddTestingFactory(
RemoteSuggestionsServiceFactory::GetInstance(),
base::BindRepeating(&BuildRemoteSuggestionsServiceWithURLLoader,
&test_url_loader_factory_));
profile_ = profile_builder.Build();
}

Expand Down Expand Up @@ -3703,22 +3717,22 @@ TEST_F(SearchProviderTest, TestDeleteMatch) {
// Test a successful deletion request.
provider_->matches_.push_back(match);
provider_->DeleteMatch(match);
EXPECT_FALSE(provider_->deletion_handlers_.empty());
EXPECT_FALSE(provider_->deletion_loaders_.empty());
EXPECT_TRUE(provider_->matches_.empty());

ASSERT_TRUE(test_url_loader_factory_.IsPending(kDeleteUrl));
test_url_loader_factory_.AddResponse(kDeleteUrl, "");

// Need to spin the event loop to let the fetch result go through.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(provider_->deletion_handlers_.empty());
EXPECT_TRUE(provider_->deletion_loaders_.empty());
EXPECT_TRUE(provider_->is_success());

// Test a failing deletion request.
test_url_loader_factory_.ClearResponses();
provider_->matches_.push_back(match);
provider_->DeleteMatch(match);
EXPECT_FALSE(provider_->deletion_handlers_.empty());
EXPECT_FALSE(provider_->deletion_loaders_.empty());
ASSERT_TRUE(test_url_loader_factory_.IsPending(kDeleteUrl));

auto head = network::mojom::URLResponseHead::New();
Expand All @@ -3730,7 +3744,7 @@ TEST_F(SearchProviderTest, TestDeleteMatch) {
network::URLLoaderCompletionStatus());

profile_->BlockUntilHistoryProcessesPendingRequests();
EXPECT_TRUE(provider_->deletion_handlers_.empty());
EXPECT_TRUE(provider_->deletion_loaders_.empty());
EXPECT_FALSE(provider_->is_success());
}

Expand Down
126 changes: 17 additions & 109 deletions components/omnibox/browser/base_search_provider.cc
View file
Expand Up @@ -19,6 +19,7 @@
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/omnibox_field_trial.h"
#include "components/omnibox/browser/remote_suggestions_service.h"
#include "components/omnibox/browser/suggestion_answer.h"
#include "components/omnibox/common/omnibox_features.h"
#include "components/search_engines/template_url.h"
Expand Down Expand Up @@ -79,107 +80,6 @@ std::u16string GetMatchContentsForOnDeviceTailSuggestion(

using OEP = metrics::OmniboxEventProto;

// SuggestionDeletionHandler -------------------------------------------------

// This class handles making requests to the server in order to delete
// personalized suggestions.
class SuggestionDeletionHandler {
public:
typedef base::OnceCallback<void(bool, SuggestionDeletionHandler*)>
DeletionCompletedCallback;

SuggestionDeletionHandler(AutocompleteProviderClient* client,
const std::string& deletion_url,
DeletionCompletedCallback callback);

~SuggestionDeletionHandler();

SuggestionDeletionHandler(const SuggestionDeletionHandler&) = delete;
SuggestionDeletionHandler& operator=(const SuggestionDeletionHandler&) =
delete;

private:
// Callback from SimpleURLLoader
void OnURLLoadComplete(const network::SimpleURLLoader* source,
std::unique_ptr<std::string> response_body);

std::unique_ptr<network::SimpleURLLoader> deletion_fetcher_;
DeletionCompletedCallback callback_;
};

SuggestionDeletionHandler::SuggestionDeletionHandler(
AutocompleteProviderClient* client,
const std::string& deletion_url,
DeletionCompletedCallback callback)
: callback_(std::move(callback)) {
GURL url(deletion_url);
DCHECK(url.is_valid());

net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("omnibox_suggest_deletion", R"(
semantics {
sender: "Omnibox"
description:
"When users attempt to delete server-provided personalized search "
"or navigation suggestions from the omnibox dropdown, Chrome sends "
"a message to the server requesting deletion of the suggestion."
trigger:
"A user attempt to delete a server-provided omnibox suggestion, "
"for which the server provided a custom deletion URL."
data:
"No user data is explicitly sent with the request, but because the "
"requested URL is provided by the server for each specific "
"suggestion, it necessarily uniquely identifies the suggestion the "
"user is attempting to delete."
destination: WEBSITE
}
policy {
cookies_allowed: YES
cookies_store: "user"
setting:
"Since this can only be triggered on seeing server-provided "
"suggestions in the omnibox dropdown, whether it is enabled is the "
"same as whether those suggestions are enabled.\n"
"Users can control this feature via the 'Use a prediction service "
"to help complete searches and URLs typed in the address bar' "
"setting under 'Privacy'. The feature is enabled by default."
chrome_policy {
SearchSuggestEnabled {
policy_options {mode: MANDATORY}
SearchSuggestEnabled: false
}
}
})");
auto request = std::make_unique<network::ResourceRequest>();
request->url = url;
variations::AppendVariationsHeaderUnknownSignedIn(
request->url,
client->IsOffTheRecord() ? variations::InIncognito::kYes
: variations::InIncognito::kNo,
request.get());
deletion_fetcher_ =
network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
deletion_fetcher_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
client->GetURLLoaderFactory().get(),
base::BindOnce(&SuggestionDeletionHandler::OnURLLoadComplete,
base::Unretained(this), deletion_fetcher_.get()));
}

SuggestionDeletionHandler::~SuggestionDeletionHandler() {
}

void SuggestionDeletionHandler::OnURLLoadComplete(
const network::SimpleURLLoader* source,
std::unique_ptr<std::string> response_body) {
DCHECK(source == deletion_fetcher_.get());
const bool ok = source->NetError() == net::OK &&
(source->ResponseInfo() && source->ResponseInfo()->headers &&
source->ResponseInfo()->headers->response_code() == 200);
std::move(callback_).Run(ok, this);
}

// BaseSearchProvider ---------------------------------------------------------

BaseSearchProvider::BaseSearchProvider(AutocompleteProvider::Type type,
AutocompleteProviderClient* client)
: AutocompleteProvider(type), client_(client) {}
Expand Down Expand Up @@ -465,10 +365,13 @@ bool BaseSearchProvider::CanSendSuggestRequestWithURL(
void BaseSearchProvider::DeleteMatch(const AutocompleteMatch& match) {
DCHECK(match.deletable);
if (!match.GetAdditionalInfo(BaseSearchProvider::kDeletionUrlKey).empty()) {
deletion_handlers_.push_back(std::make_unique<SuggestionDeletionHandler>(
client(), match.GetAdditionalInfo(BaseSearchProvider::kDeletionUrlKey),
base::BindOnce(&BaseSearchProvider::OnDeletionComplete,
base::Unretained(this))));
deletion_loaders_.push_back(
client()
->GetRemoteSuggestionsService(/*create_if_necessary=*/true)
->StartDeletionRequest(
match.GetAdditionalInfo(BaseSearchProvider::kDeletionUrlKey),
base::BindOnce(&BaseSearchProvider::OnDeletionComplete,
base::Unretained(this))));
}

const TemplateURL* template_url =
Expand Down Expand Up @@ -696,11 +599,16 @@ void BaseSearchProvider::DeleteMatchFromMatches(
}

void BaseSearchProvider::OnDeletionComplete(
bool success, SuggestionDeletionHandler* handler) {
const network::SimpleURLLoader* source,
std::unique_ptr<std::string> response_body) {
const bool success =
source->NetError() == net::OK &&
(source->ResponseInfo() && source->ResponseInfo()->headers &&
source->ResponseInfo()->headers->response_code() == 200);
RecordDeletionResult(success);
base::EraseIf(
deletion_handlers_,
[handler](const std::unique_ptr<SuggestionDeletionHandler>& elem) {
return elem.get() == handler;
deletion_loaders_,
[source](const std::unique_ptr<network::SimpleURLLoader>& loader) {
return loader.get() == source;
});
}
15 changes: 8 additions & 7 deletions components/omnibox/browser/base_search_provider.h
View file
Expand Up @@ -26,9 +26,12 @@
class AutocompleteProviderClient;
class GURL;
class SearchTermsData;
class SuggestionDeletionHandler;
class TemplateURL;

namespace network {
class SimpleURLLoader;
}

// Base functionality for receiving suggestions from a search engine.
// This class is abstract and should only be used as a base for other
// autocomplete providers utilizing its functionality.
Expand Down Expand Up @@ -176,8 +179,6 @@ class BaseSearchProvider : public AutocompleteProvider {

using MatchKey = ACMatchKey<std::u16string, std::string>;
using MatchMap = std::map<MatchKey, AutocompleteMatch>;
using SuggestionDeletionHandlers =
std::vector<std::unique_ptr<SuggestionDeletionHandler>>;

// Returns the appropriate value for the fill_into_edit field of an
// AutcompleteMatch. The result consists of the suggestion text from
Expand Down Expand Up @@ -233,15 +234,15 @@ class BaseSearchProvider : public AutocompleteProvider {
// This gets called when we have requested a suggestion deletion from the
// server to handle the results of the deletion. It will be called after the
// deletion request completes.
void OnDeletionComplete(bool success,
SuggestionDeletionHandler* handler);
void OnDeletionComplete(const network::SimpleURLLoader* source,
std::unique_ptr<std::string> response_body);

raw_ptr<AutocompleteProviderClient> client_;

// Each deletion handler in this vector corresponds to an outstanding request
// Each deletion loader in this vector corresponds to an outstanding request
// that a server delete a personalized suggestion. Making this a vector of
// unique_ptr causes us to auto-cancel all such requests on shutdown.
SuggestionDeletionHandlers deletion_handlers_;
std::vector<std::unique_ptr<network::SimpleURLLoader>> deletion_loaders_;
};

#endif // COMPONENTS_OMNIBOX_BROWSER_BASE_SEARCH_PROVIDER_H_
59 changes: 57 additions & 2 deletions components/omnibox/browser/remote_suggestions_service.cc
View file
Expand Up @@ -20,8 +20,6 @@
namespace {

void AddVariationHeaders(network::ResourceRequest* request) {
// Add Chrome experiment state to the request headers.
//
// Note: It's OK to pass InIncognito::kNo since we are expected to be in
// non-incognito state here (i.e. remote suggestions are not served in
// incognito mode).
Expand Down Expand Up @@ -107,6 +105,63 @@ RemoteSuggestionsService::StartSuggestionsRequest(
}
// Try to attach cookies for signed in user.
request->site_for_cookies = net::SiteForCookies::FromUrl(suggest_url);
// Add Chrome experiment state to the request headers.
AddVariationHeaders(request.get());

// Make loader and start download.
std::unique_ptr<network::SimpleURLLoader> loader =
network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
url_loader_factory_.get(),
base::BindOnce(std::move(completion_callback), loader.get()));
return loader;
}

std::unique_ptr<network::SimpleURLLoader>
RemoteSuggestionsService::StartDeletionRequest(
const std::string& deletion_url,
CompletionCallback completion_callback) {
const GURL url(deletion_url);
DCHECK(url.is_valid());

net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("omnibox_suggest_deletion", R"(
semantics {
sender: "Omnibox"
description:
"When users attempt to delete server-provided personalized search "
"or navigation suggestions from the omnibox dropdown, Chrome sends "
"a message to the server requesting deletion of the suggestion."
trigger:
"A user attempt to delete a server-provided omnibox suggestion, "
"for which the server provided a custom deletion URL."
data:
"No user data is explicitly sent with the request, but because the "
"requested URL is provided by the server for each specific "
"suggestion, it necessarily uniquely identifies the suggestion the "
"user is attempting to delete."
destination: WEBSITE
}
policy {
cookies_allowed: YES
cookies_store: "user"
setting:
"Since this can only be triggered on seeing server-provided "
"suggestions in the omnibox dropdown, whether it is enabled is the "
"same as whether those suggestions are enabled.\n"
"Users can control this feature via the 'Use a prediction service "
"to help complete searches and URLs typed in the address bar' "
"setting under 'Privacy'. The feature is enabled by default."
chrome_policy {
SearchSuggestEnabled {
policy_options {mode: MANDATORY}
SearchSuggestEnabled: false
}
}
})");
auto request = std::make_unique<network::ResourceRequest>();
request->url = url;
// Add Chrome experiment state to the request headers.
AddVariationHeaders(request.get());

// Make loader and start download.
Expand Down
6 changes: 6 additions & 0 deletions components/omnibox/browser/remote_suggestions_service.h
View file
Expand Up @@ -77,6 +77,12 @@ class RemoteSuggestionsService : public KeyedService {
const SearchTermsData& search_terms_data,
CompletionCallback completion_callback);

// Creates and returns a loader to delete personalized suggestions.
// `deletion_url` must be a valid URL.
std::unique_ptr<network::SimpleURLLoader> StartDeletionRequest(
const std::string& deletion_url,
CompletionCallback completion_callback);

scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
};

Expand Down
2 changes: 1 addition & 1 deletion tools/traffic_annotation/summary/annotations.xml
View file
Expand Up @@ -150,7 +150,7 @@ Refer to README.md for content description and update process.
<item id="omnibox_navigation_observer" added_in_milestone="62" content_hash_code="043a7a2f" os_list="linux,windows,chromeos" file_path="chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc" />
<item id="omnibox_result_change" added_in_milestone="62" content_hash_code="017a7557" os_list="linux,windows,chromeos,android" file_path="chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc" />
<item id="omnibox_suggest" added_in_milestone="62" content_hash_code="0524cc7e" os_list="linux,windows,chromeos,android" file_path="components/omnibox/browser/search_provider.cc" />
<item id="omnibox_suggest_deletion" added_in_milestone="62" content_hash_code="017d302e" os_list="linux,windows,chromeos,android" file_path="components/omnibox/browser/base_search_provider.cc" />
<item id="omnibox_suggest_deletion" added_in_milestone="62" content_hash_code="017d302e" os_list="linux,windows,chromeos,android" file_path="components/omnibox/browser/remote_suggestions_service.cc" />
<item id="omnibox_zerosuggest" added_in_milestone="62" content_hash_code="071e32e9" os_list="linux,windows,chromeos,android" file_path="components/omnibox/browser/remote_suggestions_service.cc" />
<item id="one_google_bar_service" added_in_milestone="62" content_hash_code="02c5f314" os_list="linux,windows,chromeos" file_path="chrome/browser/new_tab_page/one_google_bar/one_google_bar_loader_impl.cc" />
<item id="open_search" added_in_milestone="62" content_hash_code="04f2de86" os_list="linux,windows,chromeos,android" file_path="components/search_engines/template_url_fetcher.cc" />
Expand Down

0 comments on commit e53dc84

Please sign in to comment.