Merge pull request #25558 from brave/issues/41026

[ads] Fix NTP media type P3A metrics when modified from Rewards page
brave · Sep 16, 2024 · 44d1237 · 44d1237
2 parents d124791 + a673755
commit 44d1237
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 26 deletions.
diff --git a/browser/profiles/profile_util.cc b/browser/profiles/profile_util.cc
@@ -15,6 +15,7 @@
 #include "brave/components/brave_shields/core/common/brave_shield_utils.h"
 #include "brave/components/constants/brave_constants.h"
 #include "brave/components/constants/pref_names.h"
+#include "brave/components/ntp_background_images/browser/ntp_p3a_util.h"
 #include "brave/components/ntp_background_images/common/pref_names.h"
 #include "brave/components/search_engines/brave_prepopulated_engines.h"
 #include "brave/components/tor/buildflags/buildflags.h"
@@ -45,15 +46,6 @@ bool IsTorDisabledForProfile(Profile* profile) {
 #endif
 }
 
-void RecordSponsoredImagesEnabledP3A(Profile* profile) {
-  bool is_sponsored_image_enabled =
-      profile->GetPrefs()->GetBoolean(kNewTabPageShowBackgroundImage) &&
-      profile->GetPrefs()->GetBoolean(
-          kNewTabPageShowSponsoredImagesBackgroundImage);
-  UMA_HISTOGRAM_BOOLEAN("Brave.NTP.SponsoredMediaType",
-                        is_sponsored_image_enabled);
-}
-
 void RecordInitialP3AValues(Profile* profile) {
   // Preference is unregistered for some reason in profile_manager_unittest
   // TODO(bsclifton): create a proper testing profile
@@ -62,7 +54,7 @@ void RecordInitialP3AValues(Profile* profile) {
           kNewTabPageShowSponsoredImagesBackgroundImage)) {
     return;
   }
-  RecordSponsoredImagesEnabledP3A(profile);
+  ntp_background_images::RecordSponsoredImagesEnabledP3A(profile->GetPrefs());
   if (profile->IsRegularProfile()) {
     brave_shields::MaybeRecordInitialShieldsSettings(
         profile->GetPrefs(),

diff --git a/browser/profiles/profile_util.h b/browser/profiles/profile_util.h
@@ -13,10 +13,6 @@ namespace brave {
 
 bool IsTorDisabledForProfile(Profile* profile);
 
-// Specifically used to record if sponsored images are enabled.
-// Called from BraveAppearanceHandler and BraveNewTabMessageHandler
-void RecordSponsoredImagesEnabledP3A(Profile* profile);
-
 // Records default values for some histograms.
 //
 // For profile agnostic values (ex: local_state) see

diff --git a/browser/ui/webui/new_tab_page/brave_new_tab_message_handler.cc b/browser/ui/webui/new_tab_page/brave_new_tab_message_handler.cc
@@ -459,12 +459,6 @@ void BraveNewTabMessageHandler::HandleSaveNewTabPagePref(
     return;
   }
   prefs->SetBoolean(settingsKey, settingsValueBool);
-
-  // P3A can only be recorded after profile is updated
-  if (settingsKeyInput == "showBackgroundImage" ||
-      settingsKeyInput == "brandedWallpaperOptIn") {
-    brave::RecordSponsoredImagesEnabledP3A(profile_);
-  }
 }
 
 void BraveNewTabMessageHandler::HandleRegisterNewTabPageView(

diff --git a/browser/ui/webui/settings/brave_appearance_handler.cc b/browser/ui/webui/settings/brave_appearance_handler.cc
@@ -96,11 +96,6 @@ void BraveAppearanceHandler::OnBraveDarkModeChanged() {
   }
 }
 
-void BraveAppearanceHandler::OnBackgroundPreferenceChanged(
-    const std::string& pref_name) {
-  brave::RecordSponsoredImagesEnabledP3A(profile_);
-}
-
 void BraveAppearanceHandler::OnPreferenceChanged(const std::string& pref_name) {
   if (IsJavascriptAllowed()) {
     if (pref_name == kNewTabPageShowsOptions || pref_name == prefs::kHomePage ||

diff --git a/browser/ui/webui/settings/brave_appearance_handler.h b/browser/ui/webui/settings/brave_appearance_handler.h
@@ -29,7 +29,6 @@ class BraveAppearanceHandler : public settings::SettingsPageUIHandler {
   void OnJavascriptDisallowed() override {}
 
   void OnBraveDarkModeChanged();
-  void OnBackgroundPreferenceChanged(const std::string& pref_name);
   void OnPreferenceChanged(const std::string& pref_name);
   void SetBraveThemeType(const base::Value::List& args);
   void GetBraveThemeType(const base::Value::List& args);

diff --git a/components/ntp_background_images/browser/BUILD.gn b/components/ntp_background_images/browser/BUILD.gn
@@ -16,6 +16,8 @@ static_library("browser") {
     "ntp_background_images_service.cc",
     "ntp_background_images_service.h",
     "ntp_p3a_helper.h",
+    "ntp_p3a_util.cc",
+    "ntp_p3a_util.h",
     "ntp_sponsored_images_data.cc",
     "ntp_sponsored_images_data.h",
     "sponsored_images_component_data.cc",

diff --git a/components/ntp_background_images/browser/ntp_p3a_util.cc b/components/ntp_background_images/browser/ntp_p3a_util.cc
@@ -0,0 +1,24 @@
+/* Copyright (c) 2024 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/ntp_background_images/browser/ntp_p3a_util.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "brave/components/ntp_background_images/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+
+namespace ntp_background_images {
+
+void RecordSponsoredImagesEnabledP3A(const PrefService* const prefs) {
+  CHECK(prefs);
+
+  const bool is_sponsored_image_enabled =
+      prefs->GetBoolean(prefs::kNewTabPageShowBackgroundImage) &&
+      prefs->GetBoolean(prefs::kNewTabPageShowSponsoredImagesBackgroundImage);
+  UMA_HISTOGRAM_BOOLEAN("Brave.NTP.SponsoredMediaType",
+                        is_sponsored_image_enabled);
+}
+
+}  // namespace ntp_background_images
diff --git a/components/ntp_background_images/browser/ntp_p3a_util.h b/components/ntp_background_images/browser/ntp_p3a_util.h
@@ -0,0 +1,18 @@
+/* Copyright (c) 2024 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_NTP_BACKGROUND_IMAGES_BROWSER_NTP_P3A_UTIL_H_
+#define BRAVE_COMPONENTS_NTP_BACKGROUND_IMAGES_BROWSER_NTP_P3A_UTIL_H_
+
+class PrefService;
+
+namespace ntp_background_images {
+
+// Specifically used to record if sponsored images are enabled.
+void RecordSponsoredImagesEnabledP3A(const PrefService* prefs);
+
+}  // namespace ntp_background_images
+
+#endif  // BRAVE_COMPONENTS_NTP_BACKGROUND_IMAGES_BROWSER_NTP_P3A_UTIL_H_
diff --git a/components/ntp_background_images/browser/view_counter_service.cc b/components/ntp_background_images/browser/view_counter_service.cc
@@ -22,6 +22,7 @@
 #include "brave/components/ntp_background_images/browser/brave_ntp_custom_background_service.h"
 #include "brave/components/ntp_background_images/browser/ntp_background_images_data.h"
 #include "brave/components/ntp_background_images/browser/ntp_p3a_helper.h"
+#include "brave/components/ntp_background_images/browser/ntp_p3a_util.h"
 #include "brave/components/ntp_background_images/browser/ntp_sponsored_images_data.h"
 #include "brave/components/ntp_background_images/browser/url_constants.h"
 #include "brave/components/ntp_background_images/common/pref_names.h"
@@ -325,6 +326,11 @@ void ViewCounterService::OnPreferenceChanged(const std::string& pref_name) {
     return;
   }
 
+  if (pref_name == prefs::kNewTabPageShowBackgroundImage ||
+      pref_name == prefs::kNewTabPageShowSponsoredImagesBackgroundImage) {
+    RecordSponsoredImagesEnabledP3A(prefs_);
+  }
+
   // Reset model because SI and SR use different policy.
   // Start from initial model state whenever
   // prefs::kNewTabPageSuperReferralThemesOption or