Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NTP SI: update default counts and reset to initial counts on SI data update, browser restart and every 24hr #19823

Merged
merged 5 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions browser/brave_profile_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "brave/components/de_amp/common/pref_names.h"
#include "brave/components/debounce/browser/debounce_service.h"
#include "brave/components/ipfs/buildflags/buildflags.h"
#include "brave/components/ntp_background_images/browser/view_counter_service.h"
#include "brave/components/ntp_background_images/buildflags/buildflags.h"
#include "brave/components/omnibox/browser/brave_omnibox_prefs.h"
#include "brave/components/request_otr/common/buildflags/buildflags.h"
Expand Down Expand Up @@ -270,6 +271,10 @@ void RegisterProfilePrefsForMigration(
registry->RegisterListPref(pref);
}
#endif

// Added 2023-09
ntp_background_images::ViewCounterService::RegisterProfilePrefsForMigration(
registry);
}

void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
Expand Down
5 changes: 5 additions & 0 deletions chromium_src/chrome/browser/prefs/browser_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "brave/components/brave_wallet/browser/keyring_service.h"
#include "brave/components/constants/pref_names.h"
#include "brave/components/decentralized_dns/core/utils.h"
#include "brave/components/ntp_background_images/browser/view_counter_service.h"
#include "brave/components/ntp_background_images/buildflags/buildflags.h"
#include "brave/components/omnibox/browser/brave_omnibox_prefs.h"
#include "brave/components/tor/buildflags/buildflags.h"
Expand Down Expand Up @@ -229,6 +230,10 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
}
#endif

// Added 2023-09
ntp_background_images::ViewCounterService::MigrateObsoleteProfilePrefs(
profile->GetPrefs());

// END_MIGRATE_OBSOLETE_PROFILE_PREFS
}

Expand Down
4 changes: 2 additions & 2 deletions components/ntp_background_images/browser/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ BASE_DECLARE_FEATURE(kBraveNTPSuperReferralWallpaper);

BASE_DECLARE_FEATURE(kBraveNTPBrandedWallpaper);

// Show initial branded wallpaper after nth new tab page for fresh installs.
// Show initial branded wallpaper after nth new tab page for fresh opens.
constexpr base::FeatureParam<int> kInitialCountToBrandedWallpaper{
&kBraveNTPBrandedWallpaper, "initial_count_to_branded_wallpaper", 1};

// Show branded wallpaper every nth new tab page.
constexpr base::FeatureParam<int> kCountToBrandedWallpaper{
&kBraveNTPBrandedWallpaper, "count_to_branded_wallpaper", 3};
&kBraveNTPBrandedWallpaper, "count_to_branded_wallpaper", 2};
Copy link
Collaborator

@tmancey tmancey Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend leaving the default value and testing this count via this Griffin feature

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go to 2 I'm not sure I see us have the desire to go back to 3, since numbers would be worse and UX impact doesn't seem huge? Also it's less work and moving pieces in different repos syncing to when this PR hits various channels if we can combine with this PR, since we have to uplift anyway.


} // namespace features
} // namespace ntp_background_images
Expand Down
37 changes: 32 additions & 5 deletions components/ntp_background_images/browser/view_counter_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,34 @@
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include "brave/components/ntp_background_images/browser/view_counter_model.h"
#include <algorithm>

#include "base/check.h"
#include "base/logging.h"
#include "base/rand_util.h"
#include "base/time/time.h"
#include "brave/components/ntp_background_images/browser/features.h"
#include "brave/components/ntp_background_images/common/pref_names.h"
#include "components/prefs/pref_service.h"

namespace ntp_background_images {

namespace {

constexpr base::TimeDelta kCountsResetTimeDelay = base::Days(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have this configurable via Griffin, so that if the value needs changing in the future we do not need to wait for the trains


} // namespace

ViewCounterModel::ViewCounterModel(PrefService* prefs) : prefs_(prefs) {
CHECK(prefs);

count_to_branded_wallpaper_ =
prefs->GetInteger(prefs::kCountToBrandedWallpaper);
// When browser is restarted we reset to "initial" count. This will also get
// set again in the Reset() function, called e.g. when component is updated.
count_to_branded_wallpaper_ = features::kInitialCountToBrandedWallpaper.Get();

// We also reset when a specific amount of time is elapsed when in SI mode
timer_counts_reset_.Start(FROM_HERE, kCountsResetTimeDelay, this,
&ViewCounterModel::OnTimerCountsResetExpired);
}

ViewCounterModel::~ViewCounterModel() = default;
Expand Down Expand Up @@ -103,9 +116,6 @@ void ViewCounterModel::RegisterPageViewForBrandedImages() {
// Randomize campaign index for next time.
current_campaign_index_ = base::RandInt(0, total_campaign_count_ - 1);
}

prefs_->SetInteger(prefs::kCountToBrandedWallpaper,
count_to_branded_wallpaper_);
}

void ViewCounterModel::RegisterPageViewForBackgroundImages() {
Expand Down Expand Up @@ -144,6 +154,16 @@ void ViewCounterModel::IncreaseBackgroundWallpaperImageIndex() {
current_wallpaper_image_index_ %= total_image_count_;
}

void ViewCounterModel::MaybeResetBrandedWallpaperCount() {
// Set count so that user is more likely to see new branded data at least once
// Only reset count for SI images
if (!always_show_branded_wallpaper_ && show_branded_wallpaper_) {
count_to_branded_wallpaper_ =
std::min(count_to_branded_wallpaper_,
features::kInitialCountToBrandedWallpaper.Get());
}
}

void ViewCounterModel::Reset() {
current_wallpaper_image_index_ = 0;
total_image_count_ = 0;
Expand All @@ -152,6 +172,13 @@ void ViewCounterModel::Reset() {
total_campaign_count_ = 0;
campaigns_total_branded_image_count_.clear();
campaigns_current_branded_image_index_.clear();
MaybeResetBrandedWallpaperCount();
// Restart timer with same parameters as set during this class' constructor
timer_counts_reset_.Reset();
}

void ViewCounterModel::OnTimerCountsResetExpired() {
MaybeResetBrandedWallpaperCount();
}

} // namespace ntp_background_images
13 changes: 12 additions & 1 deletion components/ntp_background_images/browser/view_counter_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "base/gtest_prod_util.h"
#include "base/memory/raw_ptr.h"
#include "base/timer/timer.h"

class PrefService;

Expand Down Expand Up @@ -46,12 +47,19 @@ class ViewCounterModel {

bool ShouldShowBrandedWallpaper() const;
void RegisterPageView();
void MaybeResetBrandedWallpaperCount();
void Reset();
void IncreaseBackgroundWallpaperImageIndex();

private:
friend class NTPBackgroundImagesViewCounterTest;
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest, NTPSponsoredImagesTest);
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest,
NTPSponsoredImagesCountResetTest);
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest,
NTPSponsoredImagesCountResetMinTest);
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest,
NTPSponsoredImagesCountResetTimerTest);
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest,
NTPSponsoredImagesCountToBrandedWallpaperTest);
FRIEND_TEST_ALL_PREFIXES(ViewCounterModelTest, NTPBackgroundImagesTest);
Expand All @@ -69,15 +77,18 @@ class ViewCounterModel {

void RegisterPageViewForBackgroundImages();

void OnTimerCountsResetExpired();

// For NTP SI.
raw_ptr<PrefService> prefs_ = nullptr;
int count_to_branded_wallpaper_ = 1;
int count_to_branded_wallpaper_ = 0;
bool always_show_branded_wallpaper_ = false;
bool show_branded_wallpaper_ = true;
size_t current_campaign_index_ = 0;
size_t total_campaign_count_ = 0;
std::vector<size_t> campaigns_total_branded_image_count_;
std::vector<size_t> campaigns_current_branded_image_index_;
base::RepeatingTimer timer_counts_reset_;

// For NTP BI.
int current_wallpaper_image_index_ = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include "brave/components/ntp_background_images/browser/view_counter_model.h"
#include "base/test/scoped_feature_list.h"
#include "base/time/time.h"
#include "brave/components/ntp_background_images/browser/features.h"
#include "brave/components/ntp_background_images/browser/view_counter_service.h"
#include "brave/components/ntp_background_images/common/pref_names.h"
#include "components/sync_preferences/testing_pref_service_syncable.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace {
Expand All @@ -19,22 +22,29 @@ namespace ntp_background_images {

class ViewCounterModelTest : public testing::Test {
public:
ViewCounterModelTest() = default;
ViewCounterModelTest()
: task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
~ViewCounterModelTest() override = default;

void SetUp() override {
auto* registry = prefs()->registry();
ViewCounterService::RegisterProfilePrefs(registry);

base::FieldTrialParams parameters;
std::vector<base::test::FeatureRefAndParams> enabled_features;
parameters[features::kInitialCountToBrandedWallpaper.name] = "1";
parameters[features::kCountToBrandedWallpaper.name] = "3";
enabled_features.emplace_back(features::kBraveNTPBrandedWallpaper,
parameters);
feature_list_.InitWithFeaturesAndParameters(enabled_features, {});
}

sync_preferences::TestingPrefServiceSyncable* prefs() { return &prefs_; }

void SetCountToBrandedWallPaper(int count) {
prefs()->SetInteger(prefs::kCountToBrandedWallpaper, count);
}

protected:
content::BrowserTaskEnvironment task_environment_;
sync_preferences::TestingPrefServiceSyncable prefs_;
base::test::ScopedFeatureList feature_list_;
};

TEST_F(ViewCounterModelTest, NTPSponsoredImagesTest) {
Expand Down Expand Up @@ -82,9 +92,8 @@ TEST_F(ViewCounterModelTest, NTPSponsoredImagesTest) {
}

TEST_F(ViewCounterModelTest, NTPSponsoredImagesCountToBrandedWallpaperTest) {
SetCountToBrandedWallPaper(1);

ViewCounterModel model(prefs());
model.count_to_branded_wallpaper_ = 1;

model.SetCampaignsTotalBrandedImageCount(kTestCampaignsTotalImageCount);

Expand Down Expand Up @@ -118,6 +127,56 @@ TEST_F(ViewCounterModelTest, NTPSponsoredImagesCountToBrandedWallpaperTest) {
model.RegisterPageView();
}

TEST_F(ViewCounterModelTest, NTPSponsoredImagesCountResetTest) {
ViewCounterModel model(prefs());
model.SetCampaignsTotalBrandedImageCount(kTestCampaignsTotalImageCount);

// Verify param value for initial count was used
EXPECT_EQ(model.count_to_branded_wallpaper_, 1);
model.RegisterPageView();
EXPECT_TRUE(model.ShouldShowBrandedWallpaper());
model.RegisterPageView();
EXPECT_FALSE(model.ShouldShowBrandedWallpaper());
EXPECT_EQ(model.count_to_branded_wallpaper_, 3);

// We expect to be reset to initial count when source data updates (which
// calls Reset).
model.Reset();
EXPECT_EQ(model.count_to_branded_wallpaper_, 1);
}

TEST_F(ViewCounterModelTest, NTPSponsoredImagesCountResetMinTest) {
ViewCounterModel model(prefs());
model.SetCampaignsTotalBrandedImageCount(kTestCampaignsTotalImageCount);

// Verify param value for initial count was used
EXPECT_EQ(model.count_to_branded_wallpaper_, 1);
model.RegisterPageView();
EXPECT_TRUE(model.ShouldShowBrandedWallpaper());
EXPECT_EQ(model.count_to_branded_wallpaper_, 0);

// We expect to be reset to initial count only if count_to_branded_wallpaper_
// is higher than initial count.
model.Reset();
EXPECT_TRUE(model.ShouldShowBrandedWallpaper());
EXPECT_EQ(model.count_to_branded_wallpaper_, 0);
}

TEST_F(ViewCounterModelTest, NTPSponsoredImagesCountResetTimerTest) {
ViewCounterModel model(prefs());
model.SetCampaignsTotalBrandedImageCount(kTestCampaignsTotalImageCount);

// Verify param value for initial count was used
EXPECT_EQ(model.count_to_branded_wallpaper_, 1);
model.RegisterPageView();
EXPECT_TRUE(model.ShouldShowBrandedWallpaper());
model.RegisterPageView();
EXPECT_FALSE(model.ShouldShowBrandedWallpaper());
EXPECT_EQ(model.count_to_branded_wallpaper_, 3);
task_environment_.FastForwardBy(base::Days(1));
EXPECT_EQ(model.count_to_branded_wallpaper_, 1);
}

TEST_F(ViewCounterModelTest, NTPBackgroundImagesTest) {
ViewCounterModel model(prefs());

Expand Down
22 changes: 17 additions & 5 deletions components/ntp_background_images/browser/view_counter_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ constexpr char kSponsoredNewTabsHistogramName[] =
"Brave.NTP.SponsoredNewTabsCreated";
constexpr int kSponsoredNewTabsBuckets[] = {0, 10, 20, 30, 40, 50};

// Obsolete pref
constexpr char kObsoleteCountToBrandedWallpaperPref[] =
"brave.count_to_branded_wallpaper";

} // namespace

namespace ntp_background_images {
Expand All @@ -60,11 +64,8 @@ void ViewCounterService::RegisterLocalStatePrefs(PrefRegistrySimple* registry) {

void ViewCounterService::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterBooleanPref(
prefs::kBrandedWallpaperNotificationDismissed, false);
registry->RegisterIntegerPref(
prefs::kCountToBrandedWallpaper,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we delete prefs::kCountToBrandedWallpaper, it would be good to clear it from Preferences file.
We usually do this from brave::RegisterProfilePrefsForMigration().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed via 22c14c5

features::kInitialCountToBrandedWallpaper.Get());
registry->RegisterBooleanPref(prefs::kBrandedWallpaperNotificationDismissed,
false);
registry->RegisterBooleanPref(
prefs::kNewTabPageShowSponsoredImagesBackgroundImage, true);
// Integer type is used because this pref is used by radio button group in
Expand All @@ -75,6 +76,17 @@ void ViewCounterService::RegisterProfilePrefs(
prefs::kNewTabPageShowBackgroundImage, true);
}

void ViewCounterService::RegisterProfilePrefsForMigration(
user_prefs::PrefRegistrySyncable* registry) {
// Added 09/2023
registry->RegisterIntegerPref(kObsoleteCountToBrandedWallpaperPref, 0);
}

void ViewCounterService::MigrateObsoleteProfilePrefs(PrefService* prefs) {
// Added 09/2023
prefs->ClearPref(kObsoleteCountToBrandedWallpaperPref);
}

ViewCounterService::ViewCounterService(
NTPBackgroundImagesService* service,
BraveNTPCustomBackgroundService* custom_service,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ class ViewCounterService : public KeyedService,

static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
static void RegisterProfilePrefsForMigration(
user_prefs::PrefRegistrySyncable* registry);
static void MigrateObsoleteProfilePrefs(PrefService* prefs);

// Lets the counter know that a New Tab Page view has occured.
// This should always be called as it will evaluate whether the user has
Expand Down
1 change: 0 additions & 1 deletion components/ntp_background_images/common/pref_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace prefs {

const char kBrandedWallpaperNotificationDismissed[] =
"brave.branded_wallpaper_notification_dismissed";
const char kCountToBrandedWallpaper[] = "brave.count_to_branded_wallpaper";
const char kNewTabPageShowSponsoredImagesBackgroundImage[] =
"brave.new_tab_page.show_branded_background_image";
const char kNewTabPageSuperReferralThemesOption[] =
Expand Down
1 change: 0 additions & 1 deletion components/ntp_background_images/common/pref_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ namespace prefs {
// The one is sponsored images wallpaper and the other is super referral
// wallpaper.
extern const char kNewTabPageShowSponsoredImagesBackgroundImage[];
extern const char kCountToBrandedWallpaper[];
extern const char kNewTabPageSuperReferralThemesOption[];
extern const char kBrandedWallpaperNotificationDismissed[];
extern const char kNewTabPageShowBackgroundImage[];
Expand Down