Skip to content

Commit

Permalink
[omnibox][calc] Create ProviderStateService to cache across windows.
Browse files Browse the repository at this point in the history
Before this CL, the calc provider uses a local field `cache_` to cache
calc matches. Providers are created per window. So the `cache_` isn't
shared across windows. If you open 2 browser windows, they can show
different cached calc matches based on your previous inputs in each
window.

This CL creates a `ProviderStateService` to manage provider state that
needs to be shared across windows. The calc cache is the only state it
holds with this CL. We can move the doc and HUP caches here as well in
the future.

Also adds a 'Omnibox.NumCalculatorMatches' histogram.

Bug: 1470347
Change-Id: I0980213450864f5ecd7de022a703a3a01a0184f1
Low-Coverage-Reason: TRIVIAL_CHANGE for chrome_autocomplete_provider_client.cc & autocomplete_provider_client_impl.mm .
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4867171
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Ryan Sultanem <rsult@google.com>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215490}
  • Loading branch information
manukh authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent ea9cc41 commit 35ad23e
Show file tree
Hide file tree
Showing 24 changed files with 592 additions and 26 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ static_library("browser") {
"autocomplete/document_suggestions_service_factory.h",
"autocomplete/in_memory_url_index_factory.cc",
"autocomplete/in_memory_url_index_factory.h",
"autocomplete/provider_state_service_factory.cc",
"autocomplete/provider_state_service_factory.h",
"autocomplete/remote_suggestions_service_factory.cc",
"autocomplete/remote_suggestions_service_factory.h",
"autocomplete/shortcuts_backend_factory.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
#include "build/chromeos_buildflags.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
#include "chrome/browser/autocomplete/document_suggestions_service_factory.h"
#include "chrome/browser/autocomplete/in_memory_url_index_factory.h"
#include "chrome/browser/autocomplete/provider_state_service_factory.h"
#include "chrome/browser/autocomplete/remote_suggestions_service_factory.h"
#include "chrome/browser/autocomplete/shortcuts_backend_factory.h"
#include "chrome/browser/autocomplete/zero_suggest_cache_service_factory.h"
Expand Down Expand Up @@ -296,8 +296,7 @@ ChromeAutocompleteProviderClient::GetBuiltinsToProvideAsUserTypes() {
std::vector<std::u16string> builtins_to_provide;
builtins_to_provide.push_back(
base::ASCIIToUTF16(chrome::kChromeUIChromeURLsURL));
builtins_to_provide.push_back(
base::ASCIIToUTF16(chrome::kChromeUIFlagsURL));
builtins_to_provide.push_back(base::ASCIIToUTF16(chrome::kChromeUIFlagsURL));
#if !BUILDFLAG(IS_ANDROID)
builtins_to_provide.push_back(
base::ASCIIToUTF16(chrome::kChromeUISettingsURL));
Expand Down Expand Up @@ -331,8 +330,7 @@ signin::IdentityManager* ChromeAutocompleteProviderClient::GetIdentityManager()
AutocompleteScoringModelService*
ChromeAutocompleteProviderClient::GetAutocompleteScoringModelService() const {
#if BUILDFLAG(BUILD_WITH_TFLITE_LIB)
return AutocompleteScoringModelServiceFactory::GetInstance()->GetForProfile(
profile_);
return AutocompleteScoringModelServiceFactory::GetForProfile(profile_);
#else
return nullptr;
#endif // BUILDFLAG(BUILD_WITH_TFLITE_LIB)
Expand All @@ -341,13 +339,17 @@ ChromeAutocompleteProviderClient::GetAutocompleteScoringModelService() const {
OnDeviceTailModelService*
ChromeAutocompleteProviderClient::GetOnDeviceTailModelService() const {
#if BUILDFLAG(BUILD_WITH_TFLITE_LIB)
return OnDeviceTailModelServiceFactory::GetInstance()->GetForProfile(
profile_);
return OnDeviceTailModelServiceFactory::GetForProfile(profile_);
#else
return nullptr;
#endif // BUILDFLAG(BUILD_WITH_TFLITE_LIB)
}

ProviderStateService*
ChromeAutocompleteProviderClient::GetProviderStateService() const {
return ProviderStateServiceFactory::GetForProfile(profile_);
}

bool ChromeAutocompleteProviderClient::IsOffTheRecord() const {
return profile_->IsOffTheRecord();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class ChromeAutocompleteProviderClient : public AutocompleteProviderClient {
AutocompleteScoringModelService* GetAutocompleteScoringModelService()
const override;
OnDeviceTailModelService* GetOnDeviceTailModelService() const override;
ProviderStateService* GetProviderStateService() const override;
bool IsOffTheRecord() const override;
bool IsIncognitoProfile() const override;
bool IsGuestSession() const override;
Expand Down
40 changes: 40 additions & 0 deletions chrome/browser/autocomplete/provider_state_service_factory.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/autocomplete/provider_state_service_factory.h"

#include <memory>

#include "base/no_destructor.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_selections.h"
#include "components/omnibox/browser/provider_state_service.h"
#include "content/public/browser/browser_context.h"

// static
ProviderStateService* ProviderStateServiceFactory::GetForProfile(
Profile* profile) {
return static_cast<ProviderStateService*>(
GetInstance()->GetServiceForBrowserContext(profile, true));
}

// static
ProviderStateServiceFactory* ProviderStateServiceFactory::GetInstance() {
static base::NoDestructor<ProviderStateServiceFactory> instance;
return instance.get();
}

ProviderStateServiceFactory::ProviderStateServiceFactory()
: ProfileKeyedServiceFactory("ProviderStateService",
ProfileSelections::Builder()
.WithAshInternals(ProfileSelection::kNone)
.Build()) {}

ProviderStateServiceFactory::~ProviderStateServiceFactory() = default;

std::unique_ptr<KeyedService>
ProviderStateServiceFactory::BuildServiceInstanceForBrowserContext(
content::BrowserContext* context) const {
return std::make_unique<ProviderStateService>();
}
39 changes: 39 additions & 0 deletions chrome/browser/autocomplete/provider_state_service_factory.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_AUTOCOMPLETE_PROVIDER_STATE_SERVICE_FACTORY_H_
#define CHROME_BROWSER_AUTOCOMPLETE_PROVIDER_STATE_SERVICE_FACTORY_H_

#include "base/no_destructor.h"
#include "chrome/browser/profiles/profile_keyed_service_factory.h"

class KeyedService;
class Profile;
struct ProviderStateService;

namespace content {
class BrowserContext;
}

class ProviderStateServiceFactory : public ProfileKeyedServiceFactory {
public:
static ProviderStateService* GetForProfile(Profile* profile);
static ProviderStateServiceFactory* GetInstance();

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

private:
friend base::NoDestructor<ProviderStateServiceFactory>;

ProviderStateServiceFactory();
~ProviderStateServiceFactory() override;

// Overrides from BrowserContextKeyedServiceFactory:
std::unique_ptr<KeyedService> BuildServiceInstanceForBrowserContext(
content::BrowserContext* context) const override;
};

#endif // CHROME_BROWSER_AUTOCOMPLETE_PROVIDER_STATE_FACTORY_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/autocomplete/provider_state_service_factory.h"

#include "chrome/browser/profiles/profile_testing_helper.h"
#include "testing/gtest/include/gtest/gtest.h"

class ProviderStateServiceFactoryTest : public testing::Test {
protected:
ProfileTestingHelper profile_testing_helper_;
};

TEST_F(ProviderStateServiceFactoryTest, PrefEnabledReturnsValidService) {
profile_testing_helper_.SetUp();

EXPECT_TRUE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.regular_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.incognito_profile()));

EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.guest_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.guest_profile_otr()));

#if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.system_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.system_profile_otr()));
#endif // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)

#if BUILDFLAG(IS_CHROMEOS_ASH)
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.signin_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.signin_profile_otr()));

EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.lockscreen_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.lockscreen_profile_otr()));

EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.lockscreenapp_profile()));
EXPECT_FALSE(ProviderStateServiceFactory::GetForProfile(
profile_testing_helper_.lockscreenapp_profile_otr()));
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
#include "chrome/browser/autocomplete/document_suggestions_service_factory.h"
#include "chrome/browser/autocomplete/in_memory_url_index_factory.h"
#include "chrome/browser/autocomplete/provider_state_service_factory.h"
#include "chrome/browser/autocomplete/shortcuts_backend_factory.h"
#include "chrome/browser/autofill/autocomplete_history_manager_factory.h"
#include "chrome/browser/autofill/autofill_image_fetcher_factory.h"
Expand Down Expand Up @@ -1007,6 +1008,7 @@ void ChromeBrowserMainExtraPartsProfiles::
PromoServiceFactory::GetInstance();
#endif
ProtocolHandlerRegistryFactory::GetInstance();
ProviderStateServiceFactory::GetInstance();
PushMessagingServiceFactory::GetInstance();
#if BUILDFLAG(IS_ANDROID)
ReadingListManagerFactory::GetInstance();
Expand Down
1 change: 1 addition & 0 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -5933,6 +5933,7 @@ test("unit_tests") {
"../browser/apps/user_type_filter_unittest.cc",
"../browser/autocomplete/chrome_autocomplete_provider_client_unittest.cc",
"../browser/autocomplete/chrome_autocomplete_scheme_classifier_unittest.cc",
"../browser/autocomplete/provider_state_service_factory_unittest.cc",
"../browser/autocomplete/search_provider_unittest.cc",
"../browser/autocomplete/shortcuts_provider_extension_unittest.cc",
"../browser/autofill/automated_tests/cache_replayer_unittest.cc",
Expand Down
3 changes: 3 additions & 0 deletions components/omnibox/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ static_library("browser") {
"open_tab_provider.h",
"page_classification_functions.cc",
"page_classification_functions.h",
"provider_state_service.cc",
"provider_state_service.h",
"query_tile_provider.cc",
"query_tile_provider.h",
"remote_suggestions_service.cc",
Expand Down Expand Up @@ -766,6 +768,7 @@ source_set("unit_tests") {
"bookmark_provider_unittest.cc",
"bookmark_scoring_signals_annotator_unittest.cc",
"builtin_provider_unittest.cc",
"calculator_provider_unittest.cc",
"clipboard_provider_unittest.cc",
"document_provider_unittest.cc",
"document_suggestions_service_unittest.cc",
Expand Down
11 changes: 10 additions & 1 deletion components/omnibox/browser/autocomplete_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,14 @@ void AutocompleteController::OnProviderUpdate(

if (updated_matches || done_)
UpdateResult(false, false, true);

if (done_) {
size_t calculator_count =
base::ranges::count_if(published_result_, [](const auto& match) {
return match.type == AutocompleteMatchType::CALCULATOR;
});
UMA_HISTOGRAM_COUNTS_100("Omnibox.NumCalculatorMatches", calculator_count);
}
}

void AutocompleteController::AddProviderAndTriggeringLogs(
Expand Down Expand Up @@ -822,7 +830,8 @@ void AutocompleteController::InitializeAsyncProviders(int provider_types) {
}
if (provider_types & AutocompleteProvider::TYPE_CALCULATOR &&
search_provider_ != nullptr) {
providers_.push_back(new CalculatorProvider(this, search_provider_));
providers_.push_back(
new CalculatorProvider(provider_client_.get(), this, search_provider_));
}
}

Expand Down
4 changes: 3 additions & 1 deletion components/omnibox/browser/autocomplete_provider_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class TabMatcher;
class ZeroSuggestCacheService;
class AutocompleteScoringModelService;
class OnDeviceTailModelService;
struct ProviderStateService;

namespace bookmarks {
class BookmarkModel;
Expand All @@ -41,7 +42,7 @@ namespace history {
class HistoryService;
class URLDatabase;
class TopSites;
}
} // namespace history

namespace history_clusters {
class HistoryClustersService;
Expand Down Expand Up @@ -100,6 +101,7 @@ class AutocompleteProviderClient : public OmniboxAction::Client {
virtual AutocompleteScoringModelService* GetAutocompleteScoringModelService()
const = 0;
virtual OnDeviceTailModelService* GetOnDeviceTailModelService() const = 0;
virtual ProviderStateService* GetProviderStateService() const = 0;

// The value to use for Accept-Languages HTTP header when making an HTTP
// request.
Expand Down
37 changes: 22 additions & 15 deletions components/omnibox/browser/calculator_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@
#include "components/omnibox/browser/autocomplete_match.h"
#include "components/omnibox/browser/autocomplete_match_type.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/omnibox_feature_configs.h"
#include "components/omnibox/browser/provider_state_service.h"
#include "components/omnibox/browser/search_provider.h"

CalculatorProvider::CalculatorProvider(AutocompleteProviderListener* listener,
CalculatorProvider::CalculatorProvider(AutocompleteProviderClient* client,
AutocompleteProviderListener* listener,
SearchProvider* search_provider)
: AutocompleteProvider(AutocompleteProvider::TYPE_CALCULATOR),
client_(client),
search_provider_(search_provider) {
CHECK(search_provider_);
AddListener(listener);
Expand Down Expand Up @@ -64,11 +68,11 @@ void CalculatorProvider::Stop(bool clear_cached_results,
}

void CalculatorProvider::DeleteMatch(const AutocompleteMatch& match) {
auto it = base::ranges::find_if(cache_, [&](const auto& cached_match) {
auto it = base::ranges::find_if(Cache(), [&](const auto& cached_match) {
return cached_match.destination_url == match.destination_url;
});
if (it != cache_.end()) {
cache_.erase(it);
if (it != Cache().end()) {
Cache().erase(it);
AddMatches();
}
}
Expand Down Expand Up @@ -108,23 +112,22 @@ void CalculatorProvider::AddMatchToCache(AutocompleteMatch match) {
// As the user types out an input, e.g. '1+22+33', replace the intermediate
// matches to avoid showing all of: '1+2=3', '1+22=23', '1+22+3=26', &
// '1+22+33=56'.
if (!cache_.empty() && grew_input_) {
cache_.pop_back();
}
if (!Cache().empty() && grew_input_ && !last_calc_input_.empty())
Cache().pop_back();

// Remove duplicates to avoid a repeated match reducing cache capacity.
auto duplicate = base::ranges::find_if(
cache_, [&](const auto& cached) { return cached == match.contents; },
Cache(), [&](const auto& cached) { return cached == match.contents; },
&AutocompleteMatch::contents);
if (duplicate != cache_.end())
cache_.erase(duplicate);
if (duplicate != Cache().end())
Cache().erase(duplicate);

if (cache_.size() >
if (Cache().size() >
omnibox_feature_configs::CalcProvider::Get().max_matches) {
cache_.erase(cache_.begin());
Cache().erase(Cache().begin());
}

cache_.push_back(std::move(match));
Cache().push_back(std::move(match));
last_calc_input_ = input_;
since_last_calculator_suggestion_ = 0;
}
Expand All @@ -134,9 +137,9 @@ void CalculatorProvider::AddMatches() {
// Score sequentially so they're ranked sequentially.
// TODO(manukh) Consider enforcing hard grouping (e.g. search v URL).
int relevance = omnibox_feature_configs::CalcProvider::Get().score;
for (auto& match : cache_)
for (auto& match : Cache())
match.relevance = relevance++;
matches_ = cache_;
matches_ = Cache();
}

bool CalculatorProvider::Show() {
Expand All @@ -151,3 +154,7 @@ bool CalculatorProvider::Show() {
since_last_calculator_suggestion_ <=
omnibox_feature_configs::CalcProvider::Get().num_non_calc_inputs;
}

std::vector<AutocompleteMatch>& CalculatorProvider::Cache() {
return client_->GetProviderStateService()->calculator_provider_cache;
}

0 comments on commit 35ad23e

Please sign in to comment.