Skip to content

Commit

Permalink
[Merge 110][ZPS][Journeys] Integrate ZPS in-memory caching service wi…
Browse files Browse the repository at this point in the history
…th Journeys.

In order to overcome some of the flakiness associated with the current
web-scraping approach used to source "related searches" data presented
in the Journeys UI, this CL updates PageContentAnnotationService to
operate as an observer of ZeroSuggestCacheService, such that cached ZPS
data can then be used to obtain "related searches" data for SRP visits.

(cherry picked from commit 1bc0396)

Bug: 1378776
Change-Id: I4ab25570051212a4ac3c7efbf6a91c04cc2a9971
Low-Coverage-Reason: Correctness was verified using manual testing and follow-up CL is in the works to address test coverage (aiming to get at least implementation into M110).
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4090705
Commit-Queue: Khalid Peer <khalidpeer@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1084045}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4134486
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#124}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Khalid Peer authored and Chromium LUCI CQ committed Jan 4, 2023
1 parent 94fcf95 commit fd17cd0
Show file tree
Hide file tree
Showing 13 changed files with 175 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,10 @@ class BrowsingTopicsInternalsBrowserTest

auto page_content_annotations_service =
std::make_unique<optimization_guide::PageContentAnnotationsService>(
"en-US", optimization_guide_model_providers_.at(profile).get(),
history_service, nullptr, nullptr, base::FilePath(), nullptr,
nullptr);
nullptr, "en-US",
optimization_guide_model_providers_.at(profile).get(),
history_service, nullptr, nullptr, nullptr, base::FilePath(),
nullptr, nullptr);

page_content_annotations_service->OverridePageContentAnnotatorForTesting(
&test_page_content_annotator_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,10 @@ class BrowsingTopicsBrowserTest : public BrowsingTopicsBrowserTestBase {

auto page_content_annotations_service =
std::make_unique<optimization_guide::PageContentAnnotationsService>(
"en-US", optimization_guide_model_providers_.at(profile).get(),
history_service, nullptr, nullptr, base::FilePath(), nullptr,
nullptr);
nullptr, "en-US",
optimization_guide_model_providers_.at(profile).get(),
history_service, nullptr, nullptr, nullptr, base::FilePath(),
nullptr, nullptr);

page_content_annotations_service->OverridePageContentAnnotatorForTesting(
&test_page_content_annotator_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@

#include "chrome/browser/optimization_guide/page_content_annotations_service_factory.h"

#include <memory>

#include "base/feature_list.h"
#include "base/no_destructor.h"
#include "base/task/sequenced_task_runner.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "chrome/browser/app_mode/app_mode_utils.h"
#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
#include "chrome/browser/autocomplete/zero_suggest_cache_service_factory.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
Expand All @@ -22,6 +26,7 @@
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "components/history/core/browser/history_service.h"
#include "components/leveldb_proto/public/proto_database_provider.h"
#include "components/omnibox/browser/zero_suggest_cache_service.h"
#include "components/optimization_guide/content/browser/page_content_annotations_service.h"
#include "components/optimization_guide/core/optimization_guide_features.h"
#include "components/search_engines/template_url_service.h"
Expand Down Expand Up @@ -93,6 +98,7 @@ PageContentAnnotationsServiceFactory::PageContentAnnotationsServiceFactory()
DependsOn(OptimizationGuideKeyedServiceFactory::GetInstance());
DependsOn(HistoryServiceFactory::GetInstance());
DependsOn(TemplateURLServiceFactory::GetInstance());
DependsOn(ZeroSuggestCacheServiceFactory::GetInstance());
}

PageContentAnnotationsServiceFactory::~PageContentAnnotationsServiceFactory() =
Expand All @@ -119,11 +125,14 @@ KeyedService* PageContentAnnotationsServiceFactory::BuildServiceInstanceFor(
ServiceAccessType::IMPLICIT_ACCESS);
TemplateURLService* template_url_service =
TemplateURLServiceFactory::GetForProfile(profile);
ZeroSuggestCacheService* zero_suggest_cache_service =
ZeroSuggestCacheServiceFactory::GetForProfile(profile);
if (optimization_guide_keyed_service && history_service) {
return new optimization_guide::PageContentAnnotationsService(
std::make_unique<ChromeAutocompleteProviderClient>(profile),
g_browser_process->GetApplicationLocale(),
optimization_guide_keyed_service, history_service, template_url_service,
proto_db_provider, profile_path,
zero_suggest_cache_service, proto_db_provider, profile_path,
optimization_guide_keyed_service->GetOptimizationGuideLogger(),
base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/profiles/profile_keyed_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,8 @@ IN_PROC_BROWSER_TEST_F(ProfileKeyedServiceBrowserTest,
"WebRtcEventLogManagerKeyedService",
"WebrtcAudioPrivateEventService",
"feedback::FeedbackUploaderChrome",
"sct_reporting::Factory"
"sct_reporting::Factory",
"ZeroSuggestCacheServiceFactory",
};
// clang-format on

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ class BrowsingTopicsCalculatorTest : public testing::Test {
optimization_guide::TestOptimizationGuideModelProvider>();
page_content_annotations_service_ =
std::make_unique<optimization_guide::PageContentAnnotationsService>(
"en-US", optimization_guide_model_provider_.get(),
history_service_.get(), nullptr, nullptr, base::FilePath(), nullptr,
nullptr);
nullptr, "en-US", optimization_guide_model_provider_.get(),
history_service_.get(), nullptr, nullptr, nullptr, base::FilePath(),
nullptr, nullptr);

page_content_annotations_service_->OverridePageContentAnnotatorForTesting(
&test_page_content_annotator_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,9 @@ class BrowsingTopicsServiceImplTest
optimization_guide::TestOptimizationGuideModelProvider>();
page_content_annotations_service_ =
std::make_unique<optimization_guide::PageContentAnnotationsService>(
"en-US", optimization_guide_model_provider_.get(),
history_service_.get(), nullptr, nullptr, base::FilePath(), nullptr,
nullptr);
nullptr, "en-US", optimization_guide_model_provider_.get(),
history_service_.get(), nullptr, nullptr, nullptr, base::FilePath(),
nullptr, nullptr);

page_content_annotations_service_->OverridePageContentAnnotatorForTesting(
&test_page_content_annotator_);
Expand Down
24 changes: 24 additions & 0 deletions components/omnibox/browser/zero_suggest_cache_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

#include "base/metrics/histogram_functions.h"
#include "base/trace_event/memory_usage_estimator.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/search_suggestion_parser.h"

using CacheEntry = ZeroSuggestCacheService::CacheEntry;

Expand Down Expand Up @@ -57,6 +60,27 @@ CacheEntry::CacheEntry(const CacheEntry& entry) = default;

CacheEntry::~CacheEntry() = default;

SearchSuggestionParser::SuggestResults CacheEntry::GetSuggestResults(
const AutocompleteInput& input,
const AutocompleteProviderClient& client) const {
SearchSuggestionParser::Results results;

auto response_data =
SearchSuggestionParser::DeserializeJsonData(response_json);
if (!response_data) {
return results.suggest_results;
}

if (!SearchSuggestionParser::ParseSuggestResults(
*response_data, input, client.GetSchemeClassifier(),
/*default_result_relevance=*/100, /*is_keyword_result=*/false,
&results)) {
return results.suggest_results;
}

return results.suggest_results;
}

size_t CacheEntry::EstimateMemoryUsage() const {
return base::trace_event::EstimateMemoryUsage(response_json);
}
11 changes: 11 additions & 0 deletions components/omnibox/browser/zero_suggest_cache_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
#include "base/observer_list_types.h"
#include "build/build_config.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/search_suggestion_parser.h"

class ZeroSuggestCacheService : public KeyedService {
public:
Expand All @@ -27,6 +30,14 @@ class ZeroSuggestCacheService : public KeyedService {
// JSON response received from the remote Suggest service.
std::string response_json;

// Parses the stored JSON response in order to extract the list of
// suggestions received from the remote Suggest service.
// For memory efficiency reasons, CacheEntry does not store the
// deserialized SuggestResults object as a data member.
SearchSuggestionParser::SuggestResults GetSuggestResults(
const AutocompleteInput& input,
const AutocompleteProviderClient& client) const;

// Estimates dynamic memory usage.
// See base/trace_event/memory_usage_estimator.h for more info.
size_t EstimateMemoryUsage() const;
Expand Down
2 changes: 2 additions & 0 deletions components/optimization_guide/content/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ static_library("browser") {
"//components/history/core/browser",
"//components/keyed_service/core",
"//components/no_state_prefetch/browser",
"//components/omnibox/browser",
"//components/optimization_guide:machine_learning_tflite_buildflags",
"//components/optimization_guide:optimization_guide_buildflags",
"//components/optimization_guide/content/mojom:mojo_interfaces",
Expand All @@ -50,6 +51,7 @@ static_library("browser") {
"//services/metrics/public/cpp:metrics_cpp",
"//services/metrics/public/cpp:ukm_builders",
"//services/network/public/cpp",
"//third_party/omnibox_proto",
]
if (build_with_tflite_lib) {
public_deps += [
Expand Down
2 changes: 2 additions & 0 deletions components/optimization_guide/content/browser/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ include_rules = [
"+components/history/core/browser",
"+components/keyed_service/core",
"+components/no_state_prefetch/browser",
"+components/omnibox/browser",
"+components/optimization_guide/core",
"+components/optimization_guide/proto",
"+components/search_engines",
Expand All @@ -15,4 +16,5 @@ include_rules = [
"+services/metrics/public",
"+services/service_manager/public",
"+third_party/blink/public",
"+third_party/omnibox_proto",
]
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

#include "components/optimization_guide/content/browser/page_content_annotations_service.h"

#include <algorithm>
#include <utility>

#include "base/barrier_closure.h"
#include "base/containers/adapters.h"
#include "base/metrics/histogram_functions.h"
Expand All @@ -14,6 +17,8 @@
#include "components/history/core/browser/history_service.h"
#include "components/history/core/browser/history_types.h"
#include "components/leveldb_proto/public/proto_database_provider.h"
#include "components/omnibox/browser/autocomplete_input.h"
#include "components/omnibox/browser/search_suggestion_parser.h"
#include "components/optimization_guide/content/browser/page_content_annotations_validator.h"
#include "components/optimization_guide/core/entity_metadata.h"
#include "components/optimization_guide/core/local_page_entities_metadata_provider.h"
Expand All @@ -30,6 +35,7 @@
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "services/metrics/public/cpp/ukm_source.h"
#include "services/metrics/public/cpp/ukm_source_id.h"
#include "third_party/omnibox_proto/types.pb.h"

#if BUILDFLAG(BUILD_WITH_TFLITE_LIB)
#include "components/optimization_guide/content/browser/page_content_annotations_model_manager.h"
Expand Down Expand Up @@ -111,25 +117,33 @@ void MaybeRecordVisibilityUKM(
} // namespace

PageContentAnnotationsService::PageContentAnnotationsService(
std::unique_ptr<AutocompleteProviderClient> autocomplete_provider_client,
const std::string& application_locale,
OptimizationGuideModelProvider* optimization_guide_model_provider,
history::HistoryService* history_service,
TemplateURLService* template_url_service,
ZeroSuggestCacheService* zero_suggest_cache_service,
leveldb_proto::ProtoDatabaseProvider* database_provider,
const base::FilePath& database_dir,
OptimizationGuideLogger* optimization_guide_logger,
scoped_refptr<base::SequencedTaskRunner> background_task_runner)
: min_page_category_score_to_persist_(
: autocomplete_provider_client_(std::move(autocomplete_provider_client)),
min_page_category_score_to_persist_(
features::GetMinimumPageCategoryScoreToPersist()),
history_service_(history_service),
template_url_service_(template_url_service),
zero_suggest_cache_service_(zero_suggest_cache_service),
last_annotated_history_visits_(
features::MaxContentAnnotationRequestsCached()),
annotated_text_cache_(features::MaxVisitAnnotationCacheSize()),
optimization_guide_logger_(optimization_guide_logger) {
DCHECK(optimization_guide_model_provider);
DCHECK(history_service_);
history_service_observation_.Observe(history_service_);
if (zero_suggest_cache_service_) {
zero_suggest_cache_service_observation_.Observe(
zero_suggest_cache_service_);
}
#if BUILDFLAG(BUILD_WITH_TFLITE_LIB)
model_manager_ = std::make_unique<PageContentAnnotationsModelManager>(
optimization_guide_model_provider);
Expand Down Expand Up @@ -377,6 +391,10 @@ void PageContentAnnotationsService::RequestAndNotifyWhenModelAvailable(
void PageContentAnnotationsService::ExtractRelatedSearches(
const HistoryVisit& visit,
content::WebContents* web_contents) {
if (ShouldExtractRelatedSearchesFromZPSCache()) {
return;
}

search_result_extractor_client_.RequestData(
web_contents, {continuous_search::mojom::ResultType::kRelatedSearches},
base::BindOnce(&PageContentAnnotationsService::OnRelatedSearchesExtracted,
Expand Down Expand Up @@ -428,6 +446,64 @@ void PageContentAnnotationsService::OnPageContentAnnotated(
}
#endif

bool PageContentAnnotationsService::ShouldExtractRelatedSearchesFromZPSCache() {
return base::FeatureList::IsEnabled(
features::kExtractRelatedSearchesFromPrefetchedZPSResponse) &&
autocomplete_provider_client_ && zero_suggest_cache_service_;
}

void PageContentAnnotationsService::OnZeroSuggestResponseUpdated(
const std::string& page_url,
const ZeroSuggestCacheService::CacheEntry& response) {
if (!ShouldExtractRelatedSearchesFromZPSCache()) {
return;
}

if (page_url.empty() || !google_util::IsGoogleSearchUrl(GURL(page_url))) {
return;
}

history_service_->QueryURL(
GURL(page_url), /*want_visits=*/true,
base::BindOnce(&PageContentAnnotationsService::
ExtractRelatedSearchesFromZeroSuggestResponse,
weak_ptr_factory_.GetWeakPtr(), response),
&history_service_task_tracker_);
}

void PageContentAnnotationsService::
ExtractRelatedSearchesFromZeroSuggestResponse(
const ZeroSuggestCacheService::CacheEntry& response,
history::QueryURLResult url_result) {
if (url_result.visits.empty()) {
return;
}

AutocompleteInput input(u"", metrics::OmniboxEventProto::JOURNEYS,
autocomplete_provider_client_->GetSchemeClassifier());
auto suggest_results =
response.GetSuggestResults(input, *autocomplete_provider_client_);

std::vector<std::string> related_searches;
for (const auto& result : suggest_results) {
const auto subtypes = result.subtypes();
// Suggestions with HIVEMIND subtype are considered "related searches".
auto it = std::find(subtypes.begin(), subtypes.end(),
omnibox::SuggestSubtype::SUBTYPE_HIVEMIND);
if (it != subtypes.end()) {
related_searches.push_back(base::UTF16ToUTF8(
base::CollapseWhitespace(result.suggestion(), true)));
}
}

if (related_searches.empty()) {
return;
}

auto visit_id = url_result.visits.front().visit_id;
history_service_->AddRelatedSearchesForVisit(related_searches, visit_id);
}

void PageContentAnnotationsService::OnRelatedSearchesExtracted(
const HistoryVisit& visit,
continuous_search::SearchResultExtractorClientStatus status,
Expand Down

0 comments on commit fd17cd0

Please sign in to comment.