From b2fbe6e50026fe357dd767a99ea02fd2af15ea3d Mon Sep 17 00:00:00 2001 From: Aleksei Seren <86598290+aseren@users.noreply.github.com> Date: Tue, 3 Mar 2026 21:28:34 -0500 Subject: [PATCH] Profile attributes support for TimePeriodStorage (#34273) The PR adds ability to store TimePeriodStorage values for SERP metrics in profile attributes. --- browser/serp_metrics/BUILD.gn | 28 ++++- browser/serp_metrics/DEPS | 3 +- .../serp_metrics_time_period_store.cc | 71 +++++++++++ .../serp_metrics_time_period_store.h | 52 ++++++++ ...serp_metrics_time_period_store_unittest.cc | 99 +++++++++++++++ .../profiles/profile_attributes_entry.cc | 22 ++++ .../profiles/profile_attributes_entry.h | 12 +- components/time_period_storage/BUILD.gn | 5 + .../pref_time_period_store.cc | 60 +++++++++ .../pref_time_period_store.h | 46 +++++++ .../pref_time_period_store_unittest.cc | 116 ++++++++++++++++++ .../time_period_storage.cc | 40 +++--- .../time_period_storage/time_period_storage.h | 25 ++-- .../time_period_storage/time_period_store.h | 30 +++++ test/BUILD.gn | 1 + 15 files changed, 573 insertions(+), 37 deletions(-) create mode 100644 browser/serp_metrics/serp_metrics_time_period_store.cc create mode 100644 browser/serp_metrics/serp_metrics_time_period_store.h create mode 100644 browser/serp_metrics/serp_metrics_time_period_store_unittest.cc create mode 100644 components/time_period_storage/pref_time_period_store.cc create mode 100644 components/time_period_storage/pref_time_period_store.h create mode 100644 components/time_period_storage/pref_time_period_store_unittest.cc create mode 100644 components/time_period_storage/time_period_store.h diff --git a/browser/serp_metrics/BUILD.gn b/browser/serp_metrics/BUILD.gn index 584d0b0fda5..3532ce40dce 100644 --- a/browser/serp_metrics/BUILD.gn +++ b/browser/serp_metrics/BUILD.gn @@ -4,12 +4,19 @@ # You can obtain one at https://mozilla.org/MPL/2.0/. static_library("serp_metrics") { - public = [ "serp_metrics_tab_helper.h" ] + public = [ + "serp_metrics_tab_helper.h", + "serp_metrics_time_period_store.h", + ] - sources = [ "serp_metrics_tab_helper.cc" ] + sources = [ + "serp_metrics_tab_helper.cc", + "serp_metrics_time_period_store.cc", + ] public_deps = [ "//base", + "//brave/components/time_period_storage", "//components/search_engines", "//content/public/browser", ] @@ -22,6 +29,7 @@ static_library("serp_metrics") { "//brave/components/serp_metrics:features", "//chrome/browser:browser_process", "//chrome/browser/profiles:profile", + "//chrome/browser/profiles:profile_util", "//chrome/browser/search_engines", "//components/prefs", "//net", @@ -65,3 +73,19 @@ source_set("browser_tests") { defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] } + +source_set("unit_tests") { + testonly = true + + sources = [ "serp_metrics_time_period_store_unittest.cc" ] + + deps = [ + ":serp_metrics", + "//base", + "//chrome/browser/profiles:profile_util", + "//components/prefs:test_support", + "//content/test:test_support", + "//testing/gmock", + "//testing/gtest", + ] +} diff --git a/browser/serp_metrics/DEPS b/browser/serp_metrics/DEPS index c144ebc91c9..d88bd66e0d9 100644 --- a/browser/serp_metrics/DEPS +++ b/browser/serp_metrics/DEPS @@ -1,3 +1,4 @@ include_rules = [ - "+absl" + "+absl", + "+brave/components/time_period_storage", ] diff --git a/browser/serp_metrics/serp_metrics_time_period_store.cc b/browser/serp_metrics/serp_metrics_time_period_store.cc new file mode 100644 index 00000000000..394de973e23 --- /dev/null +++ b/browser/serp_metrics/serp_metrics_time_period_store.cc @@ -0,0 +1,71 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/serp_metrics/serp_metrics_time_period_store.h" + +#include + +#include "base/values.h" +#include "chrome/browser/profiles/profile_attributes_entry.h" +#include "chrome/browser/profiles/profile_attributes_storage.h" + +SerpMetricsTimePeriodStore::SerpMetricsTimePeriodStore( + const base::FilePath& profile_path, + ProfileAttributesStorage& profile_attributes_storage, + std::string metric_name) + : profile_path_(profile_path), + profile_attributes_storage_(profile_attributes_storage), + metric_name_(std::move(metric_name)) {} + +SerpMetricsTimePeriodStore::~SerpMetricsTimePeriodStore() = default; + +const base::ListValue* SerpMetricsTimePeriodStore::Get() { + const ProfileAttributesEntry* entry = + profile_attributes_storage_->GetProfileAttributesWithPath(profile_path_); + if (!entry) { + return nullptr; + } + + const base::DictValue* serp_metrics = entry->GetSerpMetrics(); + if (!serp_metrics) { + return nullptr; + } + + return serp_metrics->FindList(metric_name_); +} + +void SerpMetricsTimePeriodStore::Set(base::ListValue list) { + ProfileAttributesEntry* entry = + profile_attributes_storage_->GetProfileAttributesWithPath(profile_path_); + if (!entry) { + return; + } + + base::DictValue serp_metrics; + if (const base::DictValue* existing_serp_metrics = entry->GetSerpMetrics(); + existing_serp_metrics) { + serp_metrics = existing_serp_metrics->Clone(); + } + + serp_metrics.Set(metric_name_, std::move(list)); + entry->SetSerpMetrics(std::move(serp_metrics)); +} + +void SerpMetricsTimePeriodStore::Clear() { + ProfileAttributesEntry* entry = + profile_attributes_storage_->GetProfileAttributesWithPath(profile_path_); + if (!entry) { + return; + } + + const base::DictValue* serp_metrics = entry->GetSerpMetrics(); + if (!serp_metrics || !serp_metrics->contains(metric_name_)) { + return; + } + + base::DictValue mutable_serp_metrics = serp_metrics->Clone(); + mutable_serp_metrics.Remove(metric_name_); + entry->SetSerpMetrics(std::move(mutable_serp_metrics)); +} diff --git a/browser/serp_metrics/serp_metrics_time_period_store.h b/browser/serp_metrics/serp_metrics_time_period_store.h new file mode 100644 index 00000000000..b87ccc9e6c1 --- /dev/null +++ b/browser/serp_metrics/serp_metrics_time_period_store.h @@ -0,0 +1,52 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_SERP_METRICS_SERP_METRICS_TIME_PERIOD_STORE_H_ +#define BRAVE_BROWSER_SERP_METRICS_SERP_METRICS_TIME_PERIOD_STORE_H_ + +#include + +#include "base/files/file_path.h" +#include "base/memory/raw_ref.h" +#include "brave/components/time_period_storage/time_period_store.h" + +class ProfileAttributesStorage; + +namespace base { +class ListValue; +} // namespace base + +// Implementation of TimePeriodStore for SERP metrics that uses profile +// attributes for storage and has the following data structure: +// "serp_metrics": { +// "metric_name_1": [time_period_values_1], +// "metric_name_2": [time_period_values_2], +// ... +// } +class SerpMetricsTimePeriodStore : public TimePeriodStore { + public: + SerpMetricsTimePeriodStore( + const base::FilePath& profile_path, + ProfileAttributesStorage& profile_attributes_storage, + std::string metric_name); + + ~SerpMetricsTimePeriodStore() override; + + SerpMetricsTimePeriodStore(const SerpMetricsTimePeriodStore&) = delete; + SerpMetricsTimePeriodStore& operator=(const SerpMetricsTimePeriodStore&) = + delete; + + // TimePeriodStore: + const base::ListValue* Get() override; + void Set(base::ListValue list) override; + void Clear() override; + + private: + const base::FilePath profile_path_; + const raw_ref profile_attributes_storage_; + const std::string metric_name_; +}; + +#endif // BRAVE_BROWSER_SERP_METRICS_SERP_METRICS_TIME_PERIOD_STORE_H_ diff --git a/browser/serp_metrics/serp_metrics_time_period_store_unittest.cc b/browser/serp_metrics/serp_metrics_time_period_store_unittest.cc new file mode 100644 index 00000000000..a2f6d4ffd21 --- /dev/null +++ b/browser/serp_metrics/serp_metrics_time_period_store_unittest.cc @@ -0,0 +1,99 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/serp_metrics/serp_metrics_time_period_store.h" + +#include "base/files/file_path.h" +#include "chrome/browser/profiles/profile_attributes_storage.h" +#include "components/prefs/testing_pref_service.h" +#include "content/public/test/browser_task_environment.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +constexpr char kMetricName[] = "testing_metric"; +constexpr base::FilePath::CharType kUserDataDir[] = + FILE_PATH_LITERAL("/testing_user_data_dir"); + +} // namespace + +class SerpMetricsTimePeriodStoreTest : public ::testing::Test { + public: + SerpMetricsTimePeriodStoreTest() + : profile_path_( + base::FilePath(kUserDataDir).AppendASCII("testing_profile")) {} + + ~SerpMetricsTimePeriodStoreTest() override = default; + + void SetUp() override { + ProfileAttributesStorage::RegisterPrefs(local_state_.registry()); + storage_ = std::make_unique( + &local_state_, base::FilePath(kUserDataDir)); + ProfileAttributesInitParams profile_init_params; + profile_init_params.profile_path = profile_path_; + storage_->AddProfile(std::move(profile_init_params)); + } + + base::FilePath profile_path() { return profile_path_; } + + ProfileAttributesStorage& profile_attributes_storage() { return *storage_; } + + protected: + content::BrowserTaskEnvironment task_environment_; + TestingPrefServiceSimple local_state_; + const base::FilePath profile_path_; + std::unique_ptr storage_; +}; + +TEST_F(SerpMetricsTimePeriodStoreTest, SetStore) { + SerpMetricsTimePeriodStore store(profile_path(), profile_attributes_storage(), + kMetricName); + store.Set(base::ListValue().Append(1)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(1)); +} + +TEST_F(SerpMetricsTimePeriodStoreTest, UpdateStore) { + SerpMetricsTimePeriodStore store(profile_path(), profile_attributes_storage(), + kMetricName); + store.Set(base::ListValue().Append(1)); + + // Update the store with new list. + store.Set(base::ListValue().Append(2).Append(3)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(2, 3)); +} + +TEST_F(SerpMetricsTimePeriodStoreTest, ClearStore) { + SerpMetricsTimePeriodStore store(profile_path(), profile_attributes_storage(), + kMetricName); + store.Set(base::ListValue().Append(1)); + store.Clear(); + + EXPECT_FALSE(store.Get()); +} + +TEST_F(SerpMetricsTimePeriodStoreTest, GetUninitializedStore) { + SerpMetricsTimePeriodStore store(profile_path(), profile_attributes_storage(), + kMetricName); + EXPECT_FALSE(store.Get()); +} + +TEST_F(SerpMetricsTimePeriodStoreTest, SetStoresWithDifferentKeys) { + SerpMetricsTimePeriodStore store1(profile_path(), + profile_attributes_storage(), kMetricName); + SerpMetricsTimePeriodStore store2( + profile_path(), profile_attributes_storage(), "other_testing_metric"); + store1.Set(base::ListValue().Append(1)); + store2.Set(base::ListValue().Append(2).Append(3)); + + ASSERT_TRUE(store1.Get()); + EXPECT_THAT(*store1.Get(), ::testing::ElementsAre(1)); + ASSERT_TRUE(store2.Get()); + EXPECT_THAT(*store2.Get(), ::testing::ElementsAre(2, 3)); +} diff --git a/chromium_src/chrome/browser/profiles/profile_attributes_entry.cc b/chromium_src/chrome/browser/profiles/profile_attributes_entry.cc index 966f7826a6a..7125532db44 100644 --- a/chromium_src/chrome/browser/profiles/profile_attributes_entry.cc +++ b/chromium_src/chrome/browser/profiles/profile_attributes_entry.cc @@ -5,9 +5,17 @@ #include "chrome/browser/profiles/profile_attributes_entry.h" +#include +#include + #include "base/check.h" +#include "base/values.h" #include "chrome/browser/profiles/profile_avatar_icon_util.h" +namespace { +constexpr std::string_view kSerpMetricsKey = "serp_metrics"; +} // namespace + void ProfileAttributesEntry::BraveMigrateObsoleteProfileAttributes() { // Run our migrations #if !BUILDFLAG(IS_ANDROID) @@ -36,6 +44,20 @@ void ProfileAttributesEntry::BraveMigrateObsoleteProfileAttributes() { #endif } +// Retrieves a SERP metric from profile attributes. +const base::DictValue* ProfileAttributesEntry::GetSerpMetrics() const { + const base::Value* serp_metrics = GetValue(kSerpMetricsKey.data()); + if (!serp_metrics) { + return nullptr; + } + return serp_metrics->GetIfDict(); +} + +// Stores a SERP metric in profile attributes. +void ProfileAttributesEntry::SetSerpMetrics(base::DictValue serp_metrics) { + SetValue(kSerpMetricsKey.data(), base::Value(std::move(serp_metrics))); +} + #define BRAVE_PROFILE_ATTRIBUTES_ENTRY_MIGRATE_OBSOLETE_PROFILE_ATTRIBUTES \ BraveMigrateObsoleteProfileAttributes(); diff --git a/chromium_src/chrome/browser/profiles/profile_attributes_entry.h b/chromium_src/chrome/browser/profiles/profile_attributes_entry.h index c10efd70ab0..97852f1f93d 100644 --- a/chromium_src/chrome/browser/profiles/profile_attributes_entry.h +++ b/chromium_src/chrome/browser/profiles/profile_attributes_entry.h @@ -8,6 +8,10 @@ #include "build/build_config.h" +namespace base { +class DictValue; +} // namespace base + #if BUILDFLAG(IS_WIN) // To avoid conflicts with the macro from the Windows SDK... #undef GetUserName @@ -17,9 +21,11 @@ BraveMigrateObsoleteProfileAttributes(); \ void MigrateObsoleteProfileAttributes -#define RecordAccountNamesMetric \ - RecordAccountNamesMetric_UnUsed() {} \ - friend class ProfileAttributeMigrationTest; \ +#define RecordAccountNamesMetric \ + RecordAccountNamesMetric_UnUsed() {} \ + const base::DictValue* GetSerpMetrics() const; \ + void SetSerpMetrics(base::DictValue serp_metrics); \ + friend class ProfileAttributeMigrationTest; \ void RecordAccountNamesMetric #include // IWYU pragma: export diff --git a/components/time_period_storage/BUILD.gn b/components/time_period_storage/BUILD.gn index ae67f235c40..62eb11ff6b3 100644 --- a/components/time_period_storage/BUILD.gn +++ b/components/time_period_storage/BUILD.gn @@ -11,8 +11,11 @@ static_library("time_period_storage") { "iso_weekly_storage.h", "monthly_storage.cc", "monthly_storage.h", + "pref_time_period_store.cc", + "pref_time_period_store.h", "time_period_storage.cc", "time_period_storage.h", + "time_period_store.h", "weekly_event_storage.cc", "weekly_event_storage.h", "weekly_storage.cc", @@ -30,6 +33,7 @@ source_set("unit_tests") { sources = [ "daily_storage_unittest.cc", + "pref_time_period_store_unittest.cc", "time_period_storage_unittest.cc", "weekly_event_storage_unittest.cc", ] @@ -39,6 +43,7 @@ source_set("unit_tests") { "//base", "//base/test:test_support", "//components/prefs:test_support", + "//testing/gmock", "//testing/gtest", ] } diff --git a/components/time_period_storage/pref_time_period_store.cc b/components/time_period_storage/pref_time_period_store.cc new file mode 100644 index 00000000000..3ae553293a7 --- /dev/null +++ b/components/time_period_storage/pref_time_period_store.cc @@ -0,0 +1,60 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/time_period_storage/pref_time_period_store.h" + +#include + +#include "base/check.h" +#include "components/prefs/pref_service.h" +#include "components/prefs/scoped_user_pref_update.h" + +PrefTimePeriodStore::PrefTimePeriodStore(PrefService* prefs, + const char* pref_name) + : prefs_(prefs), pref_name_(pref_name) { + CHECK(prefs); + CHECK(pref_name); +} + +PrefTimePeriodStore::PrefTimePeriodStore(PrefService* prefs, + const char* pref_name, + const char* dict_key) + : prefs_(prefs), pref_name_(pref_name), dict_key_(dict_key) { + CHECK(prefs); + CHECK(pref_name); +} + +PrefTimePeriodStore::~PrefTimePeriodStore() = default; + +const base::ListValue* PrefTimePeriodStore::Get() { + const base::Value& pref_value = prefs_->GetValue(pref_name_); + + const base::ListValue* list; + if (dict_key_) { + list = pref_value.GetDict().FindList(dict_key_); + } else { + list = pref_value.GetIfList(); + } + + return list; +} + +void PrefTimePeriodStore::Set(base::ListValue list) { + if (dict_key_) { + ScopedDictPrefUpdate update(prefs_, pref_name_); + update->Set(dict_key_, std::move(list)); + } else { + prefs_->SetList(pref_name_, std::move(list)); + } +} + +void PrefTimePeriodStore::Clear() { + if (dict_key_) { + ScopedDictPrefUpdate update(prefs_, pref_name_); + update->Remove(dict_key_); + } else { + prefs_->ClearPref(pref_name_); + } +} diff --git a/components/time_period_storage/pref_time_period_store.h b/components/time_period_storage/pref_time_period_store.h new file mode 100644 index 00000000000..9e5f1aa935c --- /dev/null +++ b/components/time_period_storage/pref_time_period_store.h @@ -0,0 +1,46 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_PREF_TIME_PERIOD_STORE_H_ +#define BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_PREF_TIME_PERIOD_STORE_H_ + +#include "base/memory/raw_ptr.h" +#include "brave/components/time_period_storage/time_period_store.h" + +class PrefService; + +namespace base { +class ListValue; +} // namespace base + +// Implementation of TimePeriodStore that uses PrefService for storage. +// Supports both direct list prefs and dictionary-based list prefs. +class PrefTimePeriodStore : public TimePeriodStore { + public: + // Constructor for direct list pref store. + PrefTimePeriodStore(PrefService* prefs, const char* pref_name); + + // Constructor for dictionary-based list pref store. + PrefTimePeriodStore(PrefService* prefs, + const char* pref_name, + const char* dict_key); + + ~PrefTimePeriodStore() override; + + PrefTimePeriodStore(const PrefTimePeriodStore&) = delete; + PrefTimePeriodStore& operator=(const PrefTimePeriodStore&) = delete; + + // TimePeriodStore: + const base::ListValue* Get() override; + void Set(base::ListValue list) override; + void Clear() override; + + private: + const raw_ptr prefs_; + const char* pref_name_ = nullptr; + const char* dict_key_ = nullptr; +}; + +#endif // BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_PREF_TIME_PERIOD_STORE_H_ diff --git a/components/time_period_storage/pref_time_period_store_unittest.cc b/components/time_period_storage/pref_time_period_store_unittest.cc new file mode 100644 index 00000000000..f3f69314c14 --- /dev/null +++ b/components/time_period_storage/pref_time_period_store_unittest.cc @@ -0,0 +1,116 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/components/time_period_storage/pref_time_period_store.h" + +#include "base/values.h" +#include "components/prefs/pref_registry_simple.h" +#include "components/prefs/testing_pref_service.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +constexpr char kListPrefName[] = "testing.time_period_storage.list"; +constexpr char kDictPrefName[] = "testing.time_period_storage.dict"; +constexpr char kDictKey[] = "testing_key"; + +} // namespace + +class PrefTimePeriodStoreTest : public ::testing::Test { + public: + PrefTimePeriodStoreTest() { + pref_service_.registry()->RegisterListPref(kListPrefName); + pref_service_.registry()->RegisterDictionaryPref(kDictPrefName); + } + ~PrefTimePeriodStoreTest() override = default; + + PrefService* pref_service() { return &pref_service_; } + + private: + TestingPrefServiceSimple pref_service_; +}; + +TEST_F(PrefTimePeriodStoreTest, SetListPrefStore) { + PrefTimePeriodStore store(pref_service(), kListPrefName); + store.Set(base::ListValue().Append(1)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(1)); +} + +TEST_F(PrefTimePeriodStoreTest, SetDictPrefStore) { + PrefTimePeriodStore store(pref_service(), kDictPrefName, kDictKey); + store.Set(base::ListValue().Append(1)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(1)); +} + +TEST_F(PrefTimePeriodStoreTest, UpdateListPrefStore) { + PrefTimePeriodStore store(pref_service(), kListPrefName); + store.Set(base::ListValue().Append(1)); + + // Update the store with new list. + store.Set(base::ListValue().Append(2).Append(3)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(2, 3)); +} + +TEST_F(PrefTimePeriodStoreTest, UpdateDictPrefStore) { + PrefTimePeriodStore store(pref_service(), kDictPrefName, kDictKey); + store.Set(base::ListValue().Append(1)); + + // Update the store with new list. + store.Set(base::ListValue().Append(2).Append(3)); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::ElementsAre(2, 3)); +} + +TEST_F(PrefTimePeriodStoreTest, ClearListPrefStore) { + PrefTimePeriodStore store(pref_service(), kListPrefName); + store.Set(base::ListValue().Append(1)); + + store.Clear(); + + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::IsEmpty()); +} + +TEST_F(PrefTimePeriodStoreTest, ClearDictPrefStore) { + PrefTimePeriodStore store(pref_service(), kDictPrefName, kDictKey); + store.Set(base::ListValue().Append(1)); + + store.Clear(); + + EXPECT_FALSE(store.Get()); +} + +TEST_F(PrefTimePeriodStoreTest, GetUninitializedListPrefStore) { + PrefTimePeriodStore store(pref_service(), kListPrefName); + ASSERT_TRUE(store.Get()); + EXPECT_THAT(*store.Get(), ::testing::IsEmpty()); +} + +TEST_F(PrefTimePeriodStoreTest, GetUninitializedDictPrefStore) { + PrefTimePeriodStore store(pref_service(), kDictPrefName, kDictKey); + EXPECT_FALSE(store.Get()); +} + +TEST_F(PrefTimePeriodStoreTest, SetDictPrefStoresWithDifferentKeys) { + PrefTimePeriodStore store1(pref_service(), kDictPrefName, kDictKey); + PrefTimePeriodStore store2(pref_service(), kDictPrefName, + "other_testing_key"); + + store1.Set(base::ListValue().Append(1)); + store2.Set(base::ListValue().Append(2).Append(3)); + + ASSERT_TRUE(store1.Get()); + EXPECT_THAT(*store1.Get(), ::testing::ElementsAre(1)); + ASSERT_TRUE(store2.Get()); + EXPECT_THAT(*store2.Get(), ::testing::ElementsAre(2, 3)); +} diff --git a/components/time_period_storage/time_period_storage.cc b/components/time_period_storage/time_period_storage.cc index 363631fafa1..c8a0eb3ac28 100644 --- a/components/time_period_storage/time_period_storage.cc +++ b/components/time_period_storage/time_period_storage.cc @@ -14,8 +14,9 @@ #include "base/time/clock.h" #include "base/time/default_clock.h" #include "base/values.h" +#include "brave/components/time_period_storage/pref_time_period_store.h" +#include "brave/components/time_period_storage/time_period_store.h" #include "components/prefs/pref_service.h" -#include "components/prefs/scoped_user_pref_update.h" namespace { // Used to compensate for DST-related differences. i.e. time @@ -23,6 +24,17 @@ namespace { constexpr base::TimeDelta kPotentialDSTOffset = base::Hours(1); } // namespace +TimePeriodStorage::TimePeriodStorage(std::unique_ptr store, + size_t period_days, + bool should_offset_dst) + : clock_(std::make_unique()), + store_(std::move(store)), + period_days_(period_days), + should_offset_dst_(should_offset_dst) { + CHECK(store_); + Load(); +} + TimePeriodStorage::TimePeriodStorage(PrefService* prefs, const char* pref_name, size_t period_days, @@ -39,9 +51,7 @@ TimePeriodStorage::TimePeriodStorage(PrefService* prefs, size_t period_days, bool should_offset_dst) : clock_(std::make_unique()), - prefs_(prefs), - pref_name_(pref_name), - dict_key_(dict_key), + store_(std::make_unique(prefs, pref_name, dict_key)), period_days_(period_days), should_offset_dst_(should_offset_dst) { DCHECK(pref_name); @@ -57,9 +67,7 @@ TimePeriodStorage::TimePeriodStorage(PrefService* prefs, std::unique_ptr clock, bool should_offset_dst) : clock_(std::move(clock)), - prefs_(prefs), - pref_name_(pref_name), - dict_key_(dict_key), + store_(std::make_unique(prefs, pref_name, dict_key)), period_days_(period_days), should_offset_dst_(should_offset_dst) { DCHECK(prefs); @@ -169,7 +177,7 @@ bool TimePeriodStorage::IsOnePeriodPassed() const { void TimePeriodStorage::Clear() { daily_values_.clear(); - prefs_->ClearPref(pref_name_); + store_->Clear(); } void TimePeriodStorage::FilterToPeriod() { @@ -209,14 +217,7 @@ void TimePeriodStorage::FilterToPeriod() { void TimePeriodStorage::Load() { DCHECK(daily_values_.empty()); - const auto& pref_value = prefs_->GetValue(pref_name_); - - const base::ListValue* list; - if (dict_key_) { - list = pref_value.GetDict().FindList(dict_key_); - } else { - list = pref_value.GetIfList(); - } + const base::ListValue* list = store_->Get(); if (!list) { return; } @@ -249,10 +250,5 @@ void TimePeriodStorage::Save() { value.Set("value", static_cast(u.value)); list.Append(std::move(value)); } - if (dict_key_) { - ScopedDictPrefUpdate update(prefs_, pref_name_); - update->Set(dict_key_, std::move(list)); - } else { - prefs_->SetList(pref_name_, std::move(list)); - } + store_->Set(std::move(list)); } diff --git a/components/time_period_storage/time_period_storage.h b/components/time_period_storage/time_period_storage.h index 610746adad7..4177143749a 100644 --- a/components/time_period_storage/time_period_storage.h +++ b/components/time_period_storage/time_period_storage.h @@ -9,7 +9,6 @@ #include #include -#include "base/memory/raw_ptr.h" #include "base/time/time.h" namespace base { @@ -17,26 +16,36 @@ class Clock; } class PrefService; +class TimePeriodStore; // Mostly used by various P3A recorders - allows to track a sum of some // values added from time to time via |AddDelta| over the last predefined time -// period. Requires |pref_name| to be already registered. +// period. +// When using deprecated constructors, |pref_name| must be already registered. class TimePeriodStorage { public: - // Will use a list pref for storage. + // Will use a TimePeriodStore for storage. + TimePeriodStorage(std::unique_ptr store, + size_t period_days, + bool should_offset_dst = true); + + // Will use a list pref for storage. This is a deprecated constructor for + // backward compatibility. Use constructor with TimePeriodStore instead. TimePeriodStorage(PrefService* prefs, const char* pref_name, size_t period_days, bool should_offset_dst = true); - // Will use a list within a dictionary pref - // for storage. + // Will use a list within a dictionary pref for storage. This is a deprecated + // constructor for backward compatibility. Use constructor with + // TimePeriodStore instead. TimePeriodStorage(PrefService* prefs, const char* pref_name, const char* dict_key, size_t period_days, bool should_offset_dst = true); - // For tests. + // For tests only. Deprecated constructor for backward compatibility. Use + // constructor with TimePeriodStore instead. TimePeriodStorage(PrefService* prefs, const char* pref_name, const char* dict_key, @@ -72,9 +81,7 @@ class TimePeriodStorage { void Load(); void Save(); - const raw_ptr prefs_; - const char* pref_name_ = nullptr; - const char* dict_key_ = nullptr; + std::unique_ptr store_; size_t period_days_; const bool should_offset_dst_; diff --git a/components/time_period_storage/time_period_store.h b/components/time_period_storage/time_period_store.h new file mode 100644 index 00000000000..73c837f358d --- /dev/null +++ b/components/time_period_storage/time_period_store.h @@ -0,0 +1,30 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_TIME_PERIOD_STORE_H_ +#define BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_TIME_PERIOD_STORE_H_ + +namespace base { +class ListValue; +} // namespace base + +// An interface for classes that store list of time period values for a +// TimePeriodStorage. +class TimePeriodStore { + public: + virtual ~TimePeriodStore() = default; + + // Returns a pointer to a list of time period values. Returned pointer + // shouldn't be cached because it may be invalidated by the time it's used. + virtual const base::ListValue* Get() = 0; + + // Sets a list of time period values. + virtual void Set(base::ListValue list) = 0; + + // Clears the list of time period values. + virtual void Clear() = 0; +}; + +#endif // BRAVE_COMPONENTS_TIME_PERIOD_STORAGE_TIME_PERIOD_STORE_H_ diff --git a/test/BUILD.gn b/test/BUILD.gn index bfc49084e02..287fe94af35 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -174,6 +174,7 @@ test("brave_unit_tests") { "//brave/browser/policy/handlers:unit_tests", "//brave/browser/profiles:unit_tests", "//brave/browser/profiles:util", + "//brave/browser/serp_metrics:unit_tests", "//brave/browser/ui:unit_tests", "//brave/browser/ui/brave_tooltips", "//brave/browser/ui/startup:unit_tests",