From 8abbbfa44164cbc8bf680e384729e43f244a8089 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Thu, 8 Jun 2023 16:53:06 -0700 Subject: [PATCH 1/3] Fix TimePeriodStorage::IsOnePeriodPassed --- .../time_period_storage.cc | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/components/time_period_storage/time_period_storage.cc b/components/time_period_storage/time_period_storage.cc index e66d9ff1367e..f2c678ca5e11 100644 --- a/components/time_period_storage/time_period_storage.cc +++ b/components/time_period_storage/time_period_storage.cc @@ -134,8 +134,6 @@ uint64_t TimePeriodStorage::GetHighestValueInPeriod() const { } bool TimePeriodStorage::IsOnePeriodPassed() const { - // TODO(iefremov): This is not true 100% (if the browser was launched once - // per the time period just after installation, for example). return daily_values_.size() == period_days_; } @@ -147,13 +145,28 @@ void TimePeriodStorage::FilterToPeriod() { last_saved_midnight = daily_values_.front().day; } - if (now_midnight - last_saved_midnight > base::TimeDelta()) { + // Push daily values for new days. In loop condition, add one hour + // to now_midnight to account for DST changes. + for (base::Time day_midnight = last_saved_midnight + base::Days(1); + day_midnight <= (now_midnight + base::Hours(1)); + day_midnight += base::Days(1)) { // Day changed. Since we consider only small incoming intervals, lets just // save it with a new timestamp. - daily_values_.push_front({now_midnight, 0}); + if (last_saved_midnight.is_null()) { + // If this is a brand new list, insert one daily value + // with now_midnight... + day_midnight = now_midnight; + } + daily_values_.push_front({day_midnight, 0}); if (daily_values_.size() > period_days_) { daily_values_.pop_back(); } + if (last_saved_midnight.is_null()) { + // ...and break, so we only insert one element. We only want + // to insert multiple elements to make up for inactive days on + // existing lists, so that IsOnePeriodPassed works correctly. + break; + } } } From 6ae4573e66cd5225b79d76df6e1089c170c5f283 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Thu, 8 Jun 2023 16:56:07 -0700 Subject: [PATCH 2/3] Add vertical tab P3A metrics --- browser/brave_browser_process.h | 2 + browser/brave_browser_process_impl.cc | 13 ++ browser/brave_browser_process_impl.h | 3 + browser/brave_local_state_prefs.cc | 4 +- browser/misc_metrics/sources.gni | 7 + browser/misc_metrics/vertical_tab_metrics.cc | 189 ++++++++++++++++++ browser/misc_metrics/vertical_tab_metrics.h | 88 ++++++++ .../vertical_tab_metrics_browsertest.cc | 145 ++++++++++++++ components/misc_metrics/pref_names.cc | 6 + components/misc_metrics/pref_names.h | 3 + components/p3a/metric_names.h | 6 + test/BUILD.gn | 1 + test/base/testing_brave_browser_process.cc | 5 + test/base/testing_brave_browser_process.h | 1 + 14 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 browser/misc_metrics/vertical_tab_metrics.cc create mode 100644 browser/misc_metrics/vertical_tab_metrics.h create mode 100644 browser/misc_metrics/vertical_tab_metrics_browsertest.cc diff --git a/browser/brave_browser_process.h b/browser/brave_browser_process.h index 8e03d199e681..e86782d7a47c 100644 --- a/browser/brave_browser_process.h +++ b/browser/brave_browser_process.h @@ -65,6 +65,7 @@ class LocalhostPermissionComponent; namespace misc_metrics { class MenuMetrics; +class VerticalTabMetrics; } // namespace misc_metrics namespace request_otr { @@ -148,6 +149,7 @@ class BraveBrowserProcess { virtual brave_ads::ResourceComponent* resource_component() = 0; virtual brave::BraveFarblingService* brave_farbling_service() = 0; virtual misc_metrics::MenuMetrics* menu_metrics() = 0; + virtual misc_metrics::VerticalTabMetrics* vertical_tab_metrics() = 0; }; extern BraveBrowserProcess* g_brave_browser_process; diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc index e6563bd90825..060ba055b11b 100644 --- a/browser/brave_browser_process_impl.cc +++ b/browser/brave_browser_process_impl.cc @@ -18,6 +18,7 @@ #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/component_updater/brave_component_updater_configurator.h" #include "brave/browser/component_updater/brave_component_updater_delegate.h" +#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/net/brave_system_request_handler.h" #include "brave/browser/profiles/brave_profile_manager.h" #include "brave/browser/themes/brave_dark_mode_utils.h" @@ -142,6 +143,9 @@ BraveBrowserProcessImpl::BraveBrowserProcessImpl(StartupData* startup_data) // early initialize menu metrics menu_metrics(); + + // early initialize vertical tab metrics + vertical_tab_metrics(); } void BraveBrowserProcessImpl::Init() { @@ -533,3 +537,12 @@ misc_metrics::MenuMetrics* BraveBrowserProcessImpl::menu_metrics() { #endif return menu_metrics_.get(); } + +misc_metrics::VerticalTabMetrics* BraveBrowserProcessImpl::vertical_tab_metrics() { +#if !BUILDFLAG(IS_ANDROID) + if (!vertical_tab_metrics_) { + vertical_tab_metrics_ = std::make_unique(local_state()); + } +#endif + return vertical_tab_metrics_.get(); +} diff --git a/browser/brave_browser_process_impl.h b/browser/brave_browser_process_impl.h index 6b76482cb7ac..489ec7258d4f 100644 --- a/browser/brave_browser_process_impl.h +++ b/browser/brave_browser_process_impl.h @@ -60,6 +60,7 @@ class DebounceComponentInstaller; namespace misc_metrics { class MenuMetrics; +class VerticalTabMetrics; } // namespace misc_metrics namespace request_otr { @@ -153,6 +154,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, #endif brave::BraveFarblingService* brave_farbling_service() override; misc_metrics::MenuMetrics* menu_metrics() override; + misc_metrics::VerticalTabMetrics* vertical_tab_metrics() override; private: // BrowserProcessImpl overrides: @@ -230,6 +232,7 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, std::unique_ptr brave_farbling_service_; std::unique_ptr menu_metrics_; + std::unique_ptr vertical_tab_metrics_; std::unique_ptr brave_stats_helper_; SEQUENCE_CHECKER(sequence_checker_); diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index c549b57a2374..71db54227ddc 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -12,6 +12,7 @@ #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/metrics/buildflags/buildflags.h" #include "brave/browser/metrics/metrics_reporting_util.h" +#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/ntp_background/ntp_p3a_helper_impl.h" #include "brave/browser/playlist/playlist_service_factory.h" #include "brave/browser/themes/brave_dark_mode_utils.h" @@ -134,11 +135,12 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { ntp_background_images::NTPP3AHelperImpl::RegisterLocalStatePrefs(registry); brave_wallet::RegisterLocalStatePrefs(registry); - + misc_metrics::MenuMetrics::RegisterPrefs(registry); misc_metrics::PageMetricsService::RegisterPrefs(registry); brave_ads::BraveStatsHelper::RegisterLocalStatePrefs(registry); misc_metrics::GeneralBrowserUsage::RegisterPrefs(registry); + misc_metrics::VerticalTabMetrics::RegisterPrefs(registry); playlist::PlaylistServiceFactory::RegisterLocalStatePrefs(registry); } diff --git a/browser/misc_metrics/sources.gni b/browser/misc_metrics/sources.gni index 33a54f375eb0..8a5e5ebc9cb7 100644 --- a/browser/misc_metrics/sources.gni +++ b/browser/misc_metrics/sources.gni @@ -10,6 +10,13 @@ brave_browser_misc_metrics_sources = [ "//brave/browser/misc_metrics/page_metrics_tab_helper.h", ] +if (!is_android) { + brave_browser_misc_metrics_sources += [ + "//brave/browser/misc_metrics/vertical_tab_metrics.cc", + "//brave/browser/misc_metrics/vertical_tab_metrics.h", + ] +} + brave_browser_misc_metrics_deps = [ "//brave/browser:browser_process", "//brave/browser/profiles:util", diff --git a/browser/misc_metrics/vertical_tab_metrics.cc b/browser/misc_metrics/vertical_tab_metrics.cc new file mode 100644 index 000000000000..bc747d4ace7d --- /dev/null +++ b/browser/misc_metrics/vertical_tab_metrics.cc @@ -0,0 +1,189 @@ +/* Copyright (c) 2023 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/misc_metrics/vertical_tab_metrics.h" + +#include + +#include "brave/browser/ui/tabs/brave_tab_prefs.h" +#include "brave/components/misc_metrics/pref_names.h" +#include "brave/components/p3a_utils/bucket.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_list.h" +#include "chrome/browser/ui/tabs/tab_group_model.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "components/prefs/pref_registry_simple.h" +#include "components/prefs/pref_service.h" + +namespace misc_metrics { + +namespace { + +const int kOpenTabsBuckets[] = {1, 5, 10, 50}; +const int kGroupAndPinnedTabsBuckets[] = {2, 5}; + +const char* GetHistogramNameForCountType(TabCountType count_type) { + switch (count_type) { + case TabCountType::kOpen: + return kVerticalOpenTabsHistogramName; + case TabCountType::kGroup: + return kVerticalGroupTabsHistogramName; + case TabCountType::kPinned: + return kVerticalPinnedTabsHistogramName; + } +} + +const char* GetStoragePrefNameForCountType(TabCountType count_type) { + switch (count_type) { + case TabCountType::kOpen: + return kMiscMetricsOpenTabsStorage; + case TabCountType::kGroup: + return kMiscMetricsGroupTabsStorage; + case TabCountType::kPinned: + return kMiscMetricsPinnedTabsStorage; + } +} + +void RecordMaxToHistogramBucket(TabCountType count_type, uint64_t max_value) { + const char* histogram_name = GetHistogramNameForCountType(count_type); + switch (count_type) { + case TabCountType::kOpen: + p3a_utils::RecordToHistogramBucket(histogram_name, kOpenTabsBuckets, + max_value); + break; + case TabCountType::kGroup: + case TabCountType::kPinned: + p3a_utils::RecordToHistogramBucket(histogram_name, + kGroupAndPinnedTabsBuckets, max_value); + break; + } +} + +} // namespace + +const char kVerticalOpenTabsHistogramName[] = "Brave.VerticalTabs.OpenTabs"; +const char kVerticalGroupTabsHistogramName[] = "Brave.VerticalTabs.GroupTabs"; +const char kVerticalPinnedTabsHistogramName[] = "Brave.VerticalTabs.PinnedTabs"; + +VerticalTabBrowserMetrics::VerticalTabBrowserMetrics( + PrefService* profile_prefs, + base::RepeatingClosure change_callback) + : profile_prefs_(profile_prefs), change_callback_(change_callback) { + for (TabCountType count_type : kAllTabCountTypes) { + counts_[count_type] = 0; + } + pref_change_registrar_.Init(profile_prefs); + pref_change_registrar_.Add( + brave_tabs::kVerticalTabsEnabled, + base::BindRepeating(&VerticalTabBrowserMetrics::UpdateEnabledStatus, + base::Unretained(this))); + UpdateEnabledStatus(); +} + +VerticalTabBrowserMetrics::~VerticalTabBrowserMetrics() = default; + +void VerticalTabBrowserMetrics::OnTabStripModelChanged( + TabStripModel* tab_strip_model, + const TabStripModelChange& change, + const TabStripSelectionChange& selection) { + if (!vertical_tabs_enabled_) { + return; + } + counts_[TabCountType::kOpen] = tab_strip_model->count(); + TabGroupModel* group_model = tab_strip_model->group_model(); + if (group_model != nullptr) { + counts_[TabCountType::kGroup] = group_model->ListTabGroups().size(); + } + counts_[TabCountType::kPinned] = tab_strip_model->IndexOfFirstNonPinnedTab(); + change_callback_.Run(); +} + +bool VerticalTabBrowserMetrics::IsVerticalTabsEnabled() const { + return vertical_tabs_enabled_; +} + +size_t VerticalTabBrowserMetrics::GetTabCount(TabCountType count_type) const { + return counts_.at(count_type); +} + +void VerticalTabBrowserMetrics::UpdateEnabledStatus() { + vertical_tabs_enabled_ = + profile_prefs_->GetBoolean(brave_tabs::kVerticalTabsEnabled); +} + +VerticalTabMetrics::VerticalTabMetrics(PrefService* local_state) { + for (TabCountType count_type : kAllTabCountTypes) { + global_count_storages_[count_type] = std::make_unique( + local_state, GetStoragePrefNameForCountType(count_type)); + } + + BrowserList::GetInstance()->AddObserver(this); +} + +VerticalTabMetrics::~VerticalTabMetrics() = default; + +void VerticalTabMetrics::RegisterPrefs(PrefRegistrySimple* registry) { + for (TabCountType count_type : kAllTabCountTypes) { + registry->RegisterListPref(GetStoragePrefNameForCountType(count_type)); + } +} + +void VerticalTabMetrics::UpdateMetrics() { + bool is_vertical_tabs_enabled = false; + for (const auto& [session_id, browser_metrics] : browser_metrics_) { + if (browser_metrics->IsVerticalTabsEnabled()) { + is_vertical_tabs_enabled = true; + } + } + if (!is_vertical_tabs_enabled) { + // Do not report if vertical tabs is not enabled on any of the + // active windows/profiles. + return; + } + // Add up tab count totals from all windows + base::flat_map current_counts; + for (const auto& [session_id, browser_metrics] : browser_metrics_) { + for (TabCountType count_type : kAllTabCountTypes) { + current_counts[count_type] += browser_metrics->GetTabCount(count_type); + } + } + // Report histograms for each tab count type, if the + // particular count type is non-zero. + for (TabCountType count_type : kAllTabCountTypes) { + WeeklyStorage* storage = global_count_storages_[count_type].get(); + storage->ReplaceTodaysValueIfGreater(current_counts[count_type]); + uint64_t max_value = storage->GetHighestValueInWeek(); + if (max_value > 0) { + RecordMaxToHistogramBucket(count_type, max_value); + } + } +} + +void VerticalTabMetrics::OnBrowserAdded(Browser* browser) { + if (!browser->is_type_normal()) { + return; + } + Profile* profile = browser->profile(); + if (!profile || profile->IsOffTheRecord()) { + // Do not monitor incognito windows. + return; + } + PrefService* profile_prefs = profile->GetPrefs(); + if (!profile_prefs) { + return; + } + SessionID session_id = browser->session_id(); + browser_metrics_[session_id] = std::make_unique( + profile_prefs, base::BindRepeating(&VerticalTabMetrics::UpdateMetrics, + base::Unretained(this))); + browser->tab_strip_model()->AddObserver(browser_metrics_[session_id].get()); +} + +void VerticalTabMetrics::OnBrowserRemoved(Browser* browser) { + browser_metrics_.erase(browser->session_id()); +} + +} // namespace misc_metrics diff --git a/browser/misc_metrics/vertical_tab_metrics.h b/browser/misc_metrics/vertical_tab_metrics.h new file mode 100644 index 000000000000..1c2d60498d55 --- /dev/null +++ b/browser/misc_metrics/vertical_tab_metrics.h @@ -0,0 +1,88 @@ +/* Copyright (c) 2023 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_MISC_METRICS_VERTICAL_TAB_METRICS_H_ +#define BRAVE_BROWSER_MISC_METRICS_VERTICAL_TAB_METRICS_H_ + +#include + +#include "base/containers/flat_map.h" +#include "base/memory/raw_ptr.h" +#include "brave/components/time_period_storage/weekly_storage.h" +#include "chrome/browser/ui/browser_list_observer.h" +#include "chrome/browser/ui/tabs/tab_strip_model_observer.h" +#include "components/prefs/pref_change_registrar.h" +#include "components/sessions/core/session_id.h" + +class Browser; +class PrefRegistrySimple; +class PrefService; +class TabStripModel; + +namespace misc_metrics { + +extern const char kVerticalOpenTabsHistogramName[]; +extern const char kVerticalGroupTabsHistogramName[]; +extern const char kVerticalPinnedTabsHistogramName[]; + +enum class TabCountType { kOpen, kGroup, kPinned }; + +const TabCountType kAllTabCountTypes[] = { + TabCountType::kOpen, TabCountType::kGroup, TabCountType::kPinned}; + +class VerticalTabBrowserMetrics : public TabStripModelObserver { + public: + explicit VerticalTabBrowserMetrics(PrefService* profile_prefs, + base::RepeatingClosure change_callback); + ~VerticalTabBrowserMetrics() override; + + VerticalTabBrowserMetrics(const VerticalTabBrowserMetrics&) = delete; + VerticalTabBrowserMetrics& operator=(const VerticalTabBrowserMetrics&) = + delete; + + void OnTabStripModelChanged( + TabStripModel* tab_strip_model, + const TabStripModelChange& change, + const TabStripSelectionChange& selection) override; + + bool IsVerticalTabsEnabled() const; + size_t GetTabCount(TabCountType count_type) const; + + private: + void UpdateEnabledStatus(); + + bool vertical_tabs_enabled_; + raw_ptr profile_prefs_; + PrefChangeRegistrar pref_change_registrar_; + base::flat_map counts_; + base::RepeatingClosure change_callback_; +}; + +class VerticalTabMetrics : public BrowserListObserver { + public: + explicit VerticalTabMetrics(PrefService* local_state); + ~VerticalTabMetrics() override; + + VerticalTabMetrics(const VerticalTabMetrics&) = delete; + VerticalTabMetrics& operator=(const VerticalTabMetrics&) = delete; + + static void RegisterPrefs(PrefRegistrySimple* registry); + + void UpdateMetrics(); + + // BrowserListObserver + void OnBrowserAdded(Browser* browser) override; + void OnBrowserRemoved(Browser* browser) override; + + private: + base::flat_map> + global_count_storages_; + base::flat_map> + browser_metrics_; +}; + +} // namespace misc_metrics + +#endif // BRAVE_BROWSER_MISC_METRICS_VERTICAL_TAB_METRICS_H_ diff --git a/browser/misc_metrics/vertical_tab_metrics_browsertest.cc b/browser/misc_metrics/vertical_tab_metrics_browsertest.cc new file mode 100644 index 000000000000..d2d6a962477a --- /dev/null +++ b/browser/misc_metrics/vertical_tab_metrics_browsertest.cc @@ -0,0 +1,145 @@ +/* Copyright (c) 2023 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 "base/test/metrics/histogram_tester.h" +#include "brave/browser/misc_metrics/vertical_tab_metrics.h" +#include "brave/browser/ui/tabs/brave_tab_prefs.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_tabstrip.h" +#include "chrome/browser/ui/browser_window.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "components/prefs/pref_service.h" +#include "content/public/test/browser_test.h" + +namespace misc_metrics { + +class VerticalTabMetricsTest : public InProcessBrowserTest { + protected: + void SetVerticalTabsEnabled(bool enabled) { + return browser()->profile()->GetPrefs()->SetBoolean( + brave_tabs::kVerticalTabsEnabled, enabled); + } + + base::HistogramTester histogram_tester_; +}; + +IN_PROC_BROWSER_TEST_F(VerticalTabMetricsTest, OpenTabs) { + chrome::AddTabAt(browser(), {}, -1, true); + + histogram_tester_.ExpectTotalCount(kVerticalOpenTabsHistogramName, 0); + + SetVerticalTabsEnabled(true); + + chrome::AddTabAt(browser(), {}, -1, true); + histogram_tester_.ExpectUniqueSample(kVerticalOpenTabsHistogramName, 1, 1); + + for (size_t i = 0; i < 2; i++) { + chrome::AddTabAt(browser(), {}, -1, true); + } + histogram_tester_.ExpectUniqueSample(kVerticalOpenTabsHistogramName, 1, 3); + + chrome::AddTabAt(browser(), {}, -1, true); + histogram_tester_.ExpectBucketCount(kVerticalOpenTabsHistogramName, 2, 1); + + // Test combined counts between two windows + Browser* second_browser = CreateBrowser(browser()->profile()); + for (size_t i = 0; i < 4; i++) { + chrome::AddTabAt(second_browser, {}, -1, true); + } + histogram_tester_.ExpectBucketCount(kVerticalOpenTabsHistogramName, 2, 5); + histogram_tester_.ExpectBucketCount(kVerticalOpenTabsHistogramName, 3, 1); + + SetVerticalTabsEnabled(false); + + for (size_t i = 0; i < 3; i++) { + chrome::AddTabAt(browser(), {}, -1, true); + } + histogram_tester_.ExpectBucketCount(kVerticalOpenTabsHistogramName, 3, 1); +} + +IN_PROC_BROWSER_TEST_F(VerticalTabMetricsTest, PinnedTabs) { + chrome::AddTabAt(browser(), {}, -1, true); + + histogram_tester_.ExpectTotalCount(kVerticalPinnedTabsHistogramName, 0); + + SetVerticalTabsEnabled(true); + + for (size_t i = 0; i < 4; i++) { + chrome::AddTabAt(browser(), {}, -1, true); + } + histogram_tester_.ExpectTotalCount(kVerticalPinnedTabsHistogramName, 0); + + TabStripModel* model = browser()->tab_strip_model(); + for (size_t i = 0; i < 2; i++) { + model->SetTabPinned(i, true); + } + model->SelectNextTab(); + + histogram_tester_.ExpectUniqueSample(kVerticalPinnedTabsHistogramName, 0, 1); + + model->SetTabPinned(2, true); + model->SelectNextTab(); + + histogram_tester_.ExpectBucketCount(kVerticalPinnedTabsHistogramName, 0, 1); + histogram_tester_.ExpectBucketCount(kVerticalPinnedTabsHistogramName, 1, 1); + + for (size_t i = 0; i < 3; i++) { + model->SetTabPinned(0, false); + } + model->SelectNextTab(); + + histogram_tester_.ExpectBucketCount(kVerticalPinnedTabsHistogramName, 1, 4); + + SetVerticalTabsEnabled(false); + for (size_t i = 0; i < 3; i++) { + model->SetTabPinned(i, true); + } + model->SelectNextTab(); + histogram_tester_.ExpectTotalCount(kVerticalPinnedTabsHistogramName, 5); +} + +IN_PROC_BROWSER_TEST_F(VerticalTabMetricsTest, GroupTabs) { + chrome::AddTabAt(browser(), {}, -1, true); + + histogram_tester_.ExpectTotalCount(kVerticalGroupTabsHistogramName, 0); + + SetVerticalTabsEnabled(true); + + for (size_t i = 0; i < 4; i++) { + chrome::AddTabAt(browser(), {}, -1, true); + } + histogram_tester_.ExpectTotalCount(kVerticalGroupTabsHistogramName, 0); + + TabStripModel* model = browser()->tab_strip_model(); + for (int i = 0; i < 2; i++) { + model->AddToNewGroup({i}); + } + model->SelectNextTab(); + + histogram_tester_.ExpectUniqueSample(kVerticalGroupTabsHistogramName, 0, 1); + + model->AddToNewGroup({2}); + model->SelectNextTab(); + + histogram_tester_.ExpectBucketCount(kVerticalGroupTabsHistogramName, 0, 1); + histogram_tester_.ExpectBucketCount(kVerticalGroupTabsHistogramName, 1, 1); + + model->RemoveFromGroup({0, 1, 2}); + model->SelectNextTab(); + + histogram_tester_.ExpectBucketCount(kVerticalGroupTabsHistogramName, 1, 2); + + SetVerticalTabsEnabled(false); + + for (int i = 0; i < 3; i++) { + model->AddToNewGroup({i}); + } + model->SelectNextTab(); + histogram_tester_.ExpectTotalCount(kVerticalGroupTabsHistogramName, 3); +} + +} // namespace misc_metrics diff --git a/components/misc_metrics/pref_names.cc b/components/misc_metrics/pref_names.cc index 54144fd40611..69425a148442 100644 --- a/components/misc_metrics/pref_names.cc +++ b/components/misc_metrics/pref_names.cc @@ -14,4 +14,10 @@ const char kMiscMetricsMenuGroupActionCounts[] = const char kMiscMetricsMenuShownStorage[] = "brave.misc_metrics.menu_shown_storage"; const char kMiscMetricsPagesLoadedCount[] = "brave.core_metrics.pages_loaded"; +const char kMiscMetricsOpenTabsStorage[] = + "brave.misc_metrics.open_tabs_storage"; +const char kMiscMetricsGroupTabsStorage[] = + "brave.misc_metrics.group_tabs_storage"; +const char kMiscMetricsPinnedTabsStorage[] = + "brave.misc_metrics.pinned_tabs_storage"; } // namespace misc_metrics diff --git a/components/misc_metrics/pref_names.h b/components/misc_metrics/pref_names.h index 5b0ded39dac9..eda6310cb500 100644 --- a/components/misc_metrics/pref_names.h +++ b/components/misc_metrics/pref_names.h @@ -12,6 +12,9 @@ extern const char kMiscMetricsMenuDismissStorage[]; extern const char kMiscMetricsMenuGroupActionCounts[]; extern const char kMiscMetricsMenuShownStorage[]; extern const char kMiscMetricsPagesLoadedCount[]; +extern const char kMiscMetricsOpenTabsStorage[]; +extern const char kMiscMetricsGroupTabsStorage[]; +extern const char kMiscMetricsPinnedTabsStorage[]; } // namespace misc_metrics #endif // BRAVE_COMPONENTS_MISC_METRICS_PREF_NAMES_H_ diff --git a/components/p3a/metric_names.h b/components/p3a/metric_names.h index 731a0a9640b9..345a7c275b08 100644 --- a/components/p3a/metric_names.h +++ b/components/p3a/metric_names.h @@ -86,6 +86,9 @@ constexpr inline auto kCollectedTypicalHistograms = "Brave.Sync.Status.2", "Brave.Sync.ProgressTokenEverReset", "Brave.Uptime.BrowserOpenMinutes", + "Brave.VerticalTabs.GroupTabs", + "Brave.VerticalTabs.OpenTabs", + "Brave.VerticalTabs.PinnedTabs", "Brave.VPN.NewUserReturning", "Brave.VPN.DaysInMonthUsed", "Brave.VPN.LastUsageTime", @@ -216,6 +219,9 @@ constexpr inline auto kEphemeralHistograms = "Brave.Today.IsEnabled", "Brave.Today.UsageDaily", "Brave.Today.UsageMonthly", + "Brave.VerticalTabs.GroupTabs", + "Brave.VerticalTabs.OpenTabs", + "Brave.VerticalTabs.PinnedTabs", "Brave.Wallet.UsageDaily", "Brave.Wallet.UsageMonthly", "Brave.Wallet.UsageWeekly" diff --git a/test/BUILD.gn b/test/BUILD.gn index 2d277cf989d1..1a91ac535f88 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -1089,6 +1089,7 @@ test("brave_browser_tests") { if (!is_android) { sources += [ "//brave/browser/brave_resources_browsertest.cc", + "//brave/browser/misc_metrics/vertical_tab_metrics_browsertest.cc", "//brave/browser/ssl/certificate_transparency_browsertest.cc", "//brave/browser/ui/views/toolbar/wallet_button_notification_source_browsertest.cc", ] diff --git a/test/base/testing_brave_browser_process.cc b/test/base/testing_brave_browser_process.cc index 43f102ec623f..a06cbaa97db4 100644 --- a/test/base/testing_brave_browser_process.cc +++ b/test/base/testing_brave_browser_process.cc @@ -186,6 +186,11 @@ misc_metrics::MenuMetrics* TestingBraveBrowserProcess::menu_metrics() { return nullptr; } +misc_metrics::VerticalTabMetrics* TestingBraveBrowserProcess::vertical_tab_metrics() { + NOTREACHED(); + return nullptr; +} + void TestingBraveBrowserProcess::SetAdBlockService( std::unique_ptr service) { ad_block_service_ = std::move(service); diff --git a/test/base/testing_brave_browser_process.h b/test/base/testing_brave_browser_process.h index 34ac9c7af906..331f6eb2df2e 100644 --- a/test/base/testing_brave_browser_process.h +++ b/test/base/testing_brave_browser_process.h @@ -87,6 +87,7 @@ class TestingBraveBrowserProcess : public BraveBrowserProcess { brave_vpn::BraveVPNOSConnectionAPI* brave_vpn_os_connection_api() override; #endif misc_metrics::MenuMetrics* menu_metrics() override; + misc_metrics::VerticalTabMetrics* vertical_tab_metrics() override; // Populate the mock process with services. Consumer is responsible for // cleaning these up after completion of a test. From 0cc8d6c684dcc169ea42ae65cb09c94d80969622 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Mon, 19 Jun 2023 15:58:22 -0700 Subject: [PATCH 3/3] Vertical tab P3A improvements as per feedback --- browser/brave_browser_process.h | 4 ++++ browser/brave_browser_process_impl.cc | 12 +++++++--- browser/brave_browser_process_impl.h | 6 +++++ browser/brave_local_state_prefs.cc | 6 +++-- browser/misc_metrics/vertical_tab_metrics.cc | 24 ++++---------------- browser/misc_metrics/vertical_tab_metrics.h | 10 +++++--- test/base/testing_brave_browser_process.cc | 5 +++- test/base/testing_brave_browser_process.h | 2 ++ 8 files changed, 41 insertions(+), 28 deletions(-) diff --git a/browser/brave_browser_process.h b/browser/brave_browser_process.h index e86782d7a47c..864becb70d61 100644 --- a/browser/brave_browser_process.h +++ b/browser/brave_browser_process.h @@ -65,7 +65,9 @@ class LocalhostPermissionComponent; namespace misc_metrics { class MenuMetrics; +#if !BUILDFLAG(IS_ANDROID) class VerticalTabMetrics; +#endif } // namespace misc_metrics namespace request_otr { @@ -149,7 +151,9 @@ class BraveBrowserProcess { virtual brave_ads::ResourceComponent* resource_component() = 0; virtual brave::BraveFarblingService* brave_farbling_service() = 0; virtual misc_metrics::MenuMetrics* menu_metrics() = 0; +#if !BUILDFLAG(IS_ANDROID) virtual misc_metrics::VerticalTabMetrics* vertical_tab_metrics() = 0; +#endif }; extern BraveBrowserProcess* g_brave_browser_process; diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc index 060ba055b11b..f1635026cdc6 100644 --- a/browser/brave_browser_process_impl.cc +++ b/browser/brave_browser_process_impl.cc @@ -18,7 +18,6 @@ #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/component_updater/brave_component_updater_configurator.h" #include "brave/browser/component_updater/brave_component_updater_delegate.h" -#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/net/brave_system_request_handler.h" #include "brave/browser/profiles/brave_profile_manager.h" #include "brave/browser/themes/brave_dark_mode_utils.h" @@ -85,6 +84,7 @@ #if BUILDFLAG(IS_ANDROID) #include "chrome/browser/flags/android/chrome_feature_list.h" #else +#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/ui/brave_browser_command_controller.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_list.h" @@ -144,8 +144,10 @@ BraveBrowserProcessImpl::BraveBrowserProcessImpl(StartupData* startup_data) // early initialize menu metrics menu_metrics(); +#if !BUILDFLAG(IS_ANDROID) // early initialize vertical tab metrics vertical_tab_metrics(); +#endif } void BraveBrowserProcessImpl::Init() { @@ -538,11 +540,15 @@ misc_metrics::MenuMetrics* BraveBrowserProcessImpl::menu_metrics() { return menu_metrics_.get(); } -misc_metrics::VerticalTabMetrics* BraveBrowserProcessImpl::vertical_tab_metrics() { +#if !BUILDFLAG(IS_ANDROID) +misc_metrics::VerticalTabMetrics* +BraveBrowserProcessImpl::vertical_tab_metrics() { #if !BUILDFLAG(IS_ANDROID) if (!vertical_tab_metrics_) { - vertical_tab_metrics_ = std::make_unique(local_state()); + vertical_tab_metrics_ = + std::make_unique(local_state()); } #endif return vertical_tab_metrics_.get(); } +#endif diff --git a/browser/brave_browser_process_impl.h b/browser/brave_browser_process_impl.h index 489ec7258d4f..9843c769ae2e 100644 --- a/browser/brave_browser_process_impl.h +++ b/browser/brave_browser_process_impl.h @@ -60,7 +60,9 @@ class DebounceComponentInstaller; namespace misc_metrics { class MenuMetrics; +#if !BUILDFLAG(IS_ANDROID) class VerticalTabMetrics; +#endif } // namespace misc_metrics namespace request_otr { @@ -154,7 +156,9 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, #endif brave::BraveFarblingService* brave_farbling_service() override; misc_metrics::MenuMetrics* menu_metrics() override; +#if !BUILDFLAG(IS_ANDROID) misc_metrics::VerticalTabMetrics* vertical_tab_metrics() override; +#endif private: // BrowserProcessImpl overrides: @@ -232,7 +236,9 @@ class BraveBrowserProcessImpl : public BraveBrowserProcess, std::unique_ptr brave_farbling_service_; std::unique_ptr menu_metrics_; +#if !BUILDFLAG(IS_ANDROID) std::unique_ptr vertical_tab_metrics_; +#endif std::unique_ptr brave_stats_helper_; SEQUENCE_CHECKER(sequence_checker_); diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index 71db54227ddc..accbf0fe4399 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -12,7 +12,6 @@ #include "brave/browser/brave_stats/brave_stats_updater.h" #include "brave/browser/metrics/buildflags/buildflags.h" #include "brave/browser/metrics/metrics_reporting_util.h" -#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/ntp_background/ntp_p3a_helper_impl.h" #include "brave/browser/playlist/playlist_service_factory.h" #include "brave/browser/themes/brave_dark_mode_utils.h" @@ -46,6 +45,7 @@ #include "brave/browser/ui/webui/new_tab_page/brave_new_tab_message_handler.h" #if !BUILDFLAG(IS_ANDROID) +#include "brave/browser/misc_metrics/vertical_tab_metrics.h" #include "brave/browser/p3a/p3a_core_metrics.h" #include "brave/browser/ui/whats_new/whats_new_util.h" #include "chrome/browser/first_run/first_run.h" @@ -135,12 +135,14 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { ntp_background_images::NTPP3AHelperImpl::RegisterLocalStatePrefs(registry); brave_wallet::RegisterLocalStatePrefs(registry); - + misc_metrics::MenuMetrics::RegisterPrefs(registry); misc_metrics::PageMetricsService::RegisterPrefs(registry); brave_ads::BraveStatsHelper::RegisterLocalStatePrefs(registry); misc_metrics::GeneralBrowserUsage::RegisterPrefs(registry); +#if !BUILDFLAG(IS_ANDROID) misc_metrics::VerticalTabMetrics::RegisterPrefs(registry); +#endif playlist::PlaylistServiceFactory::RegisterLocalStatePrefs(registry); } diff --git a/browser/misc_metrics/vertical_tab_metrics.cc b/browser/misc_metrics/vertical_tab_metrics.cc index bc747d4ace7d..063f94f954e6 100644 --- a/browser/misc_metrics/vertical_tab_metrics.cc +++ b/browser/misc_metrics/vertical_tab_metrics.cc @@ -101,11 +101,10 @@ void VerticalTabBrowserMetrics::OnTabStripModelChanged( change_callback_.Run(); } -bool VerticalTabBrowserMetrics::IsVerticalTabsEnabled() const { - return vertical_tabs_enabled_; -} - size_t VerticalTabBrowserMetrics::GetTabCount(TabCountType count_type) const { + if (!vertical_tabs_enabled_) { + return 0; + } return counts_.at(count_type); } @@ -132,17 +131,6 @@ void VerticalTabMetrics::RegisterPrefs(PrefRegistrySimple* registry) { } void VerticalTabMetrics::UpdateMetrics() { - bool is_vertical_tabs_enabled = false; - for (const auto& [session_id, browser_metrics] : browser_metrics_) { - if (browser_metrics->IsVerticalTabsEnabled()) { - is_vertical_tabs_enabled = true; - } - } - if (!is_vertical_tabs_enabled) { - // Do not report if vertical tabs is not enabled on any of the - // active windows/profiles. - return; - } // Add up tab count totals from all windows base::flat_map current_counts; for (const auto& [session_id, browser_metrics] : browser_metrics_) { @@ -167,14 +155,12 @@ void VerticalTabMetrics::OnBrowserAdded(Browser* browser) { return; } Profile* profile = browser->profile(); - if (!profile || profile->IsOffTheRecord()) { + if (!profile || profile->IsOffTheRecord() || !profile->IsRegularProfile()) { // Do not monitor incognito windows. return; } PrefService* profile_prefs = profile->GetPrefs(); - if (!profile_prefs) { - return; - } + CHECK(profile_prefs); SessionID session_id = browser->session_id(); browser_metrics_[session_id] = std::make_unique( profile_prefs, base::BindRepeating(&VerticalTabMetrics::UpdateMetrics, diff --git a/browser/misc_metrics/vertical_tab_metrics.h b/browser/misc_metrics/vertical_tab_metrics.h index 1c2d60498d55..6046da31074a 100644 --- a/browser/misc_metrics/vertical_tab_metrics.h +++ b/browser/misc_metrics/vertical_tab_metrics.h @@ -27,7 +27,11 @@ extern const char kVerticalOpenTabsHistogramName[]; extern const char kVerticalGroupTabsHistogramName[]; extern const char kVerticalPinnedTabsHistogramName[]; -enum class TabCountType { kOpen, kGroup, kPinned }; +enum class TabCountType { + kOpen, + kGroup, + kPinned, +}; const TabCountType kAllTabCountTypes[] = { TabCountType::kOpen, TabCountType::kGroup, TabCountType::kPinned}; @@ -42,12 +46,12 @@ class VerticalTabBrowserMetrics : public TabStripModelObserver { VerticalTabBrowserMetrics& operator=(const VerticalTabBrowserMetrics&) = delete; + // TabStripModelObserver: void OnTabStripModelChanged( TabStripModel* tab_strip_model, const TabStripModelChange& change, const TabStripSelectionChange& selection) override; - bool IsVerticalTabsEnabled() const; size_t GetTabCount(TabCountType count_type) const; private: @@ -72,7 +76,7 @@ class VerticalTabMetrics : public BrowserListObserver { void UpdateMetrics(); - // BrowserListObserver + // BrowserListObserver: void OnBrowserAdded(Browser* browser) override; void OnBrowserRemoved(Browser* browser) override; diff --git a/test/base/testing_brave_browser_process.cc b/test/base/testing_brave_browser_process.cc index a06cbaa97db4..6d31019b61c1 100644 --- a/test/base/testing_brave_browser_process.cc +++ b/test/base/testing_brave_browser_process.cc @@ -186,10 +186,13 @@ misc_metrics::MenuMetrics* TestingBraveBrowserProcess::menu_metrics() { return nullptr; } -misc_metrics::VerticalTabMetrics* TestingBraveBrowserProcess::vertical_tab_metrics() { +#if !BUILDFLAG(IS_ANDROID) +misc_metrics::VerticalTabMetrics* +TestingBraveBrowserProcess::vertical_tab_metrics() { NOTREACHED(); return nullptr; } +#endif void TestingBraveBrowserProcess::SetAdBlockService( std::unique_ptr service) { diff --git a/test/base/testing_brave_browser_process.h b/test/base/testing_brave_browser_process.h index 331f6eb2df2e..b533f1fc3414 100644 --- a/test/base/testing_brave_browser_process.h +++ b/test/base/testing_brave_browser_process.h @@ -87,7 +87,9 @@ class TestingBraveBrowserProcess : public BraveBrowserProcess { brave_vpn::BraveVPNOSConnectionAPI* brave_vpn_os_connection_api() override; #endif misc_metrics::MenuMetrics* menu_metrics() override; +#if !BUILDFLAG(IS_ANDROID) misc_metrics::VerticalTabMetrics* vertical_tab_metrics() override; +#endif // Populate the mock process with services. Consumer is responsible for // cleaning these up after completion of a test.