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

petemill · 2023-08-22T22:41:59Z

NTP SI count defaults updated to 1st view after install and every 3rd thereafter. Previously it was 2nd view after install and every 4th thereafter. In addition, the initial view count (now 1st) is used after browser restart and after the SI data gets updated. This avoids the problem of missing the SI data for the 1st pageview after install.

Lastly, the count gets reset to the "initial" value every 24 hours. This ensures a user is likely to see a multi-day campaign each day even if they open fewer NTPs than the regular count-to-branded-wallpaper.

Resolves brave/brave-browser#31551
Resolves brave/brave-browser#33228

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

tmancey · 2023-08-23T13:47:23Z

components/ntp_background_images/browser/features.h

 constexpr base::FeatureParam<int> kInitialCountToBrandedWallpaper{
-    &kBraveNTPBrandedWallpaper, "initial_count_to_branded_wallpaper", 1};
+    &kBraveNTPBrandedWallpaper, "initial_count_to_branded_wallpaper", 0};


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

The griffin value won't apply to first run. A restart is required. But seems we may separate first-run value from after-restart initial values.

tmancey · 2023-08-23T13:47:28Z

components/ntp_background_images/browser/features.h


 // 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};


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

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.

simonhong · 2023-08-24T04:33:05Z

components/ntp_background_images/browser/view_counter_model.cc

@@ -17,8 +17,10 @@ namespace ntp_background_images {
 ViewCounterModel::ViewCounterModel(PrefService* prefs) : prefs_(prefs) {
  CHECK(prefs);

+  // When browser is restarted or component is updated, we reset to "initial"
+  // count.
  count_to_branded_wallpaper_ =


I think we have to reset this value from ViewCounterModel::Reset().
It's called whenever component is updated.

simonhong · 2023-08-24T04:36:40Z

components/ntp_background_images/browser/view_counter_service.cc

-  registry->RegisterBooleanPref(
-      prefs::kBrandedWallpaperNotificationDismissed, false);
-  registry->RegisterIntegerPref(
-      prefs::kCountToBrandedWallpaper,


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

fixed via 22c14c5

tmancey · 2023-08-24T15:32:13Z

components/ntp_background_images/browser/view_counter_model.cc

 #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);


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

simonhong · 2023-08-28T04:46:56Z

I pushed another PR #19896.
As this PR will set zero to kInitialCountToBrandedWallpaper at startup,
brave/brave-browser#32359 could be happened more frequently.
It would be good to handle it before merging this PR.

simonhong · 2023-08-29T13:02:27Z

Found another NTP issue(brave/brave-browser#32577) and I think it also could happen easily after this PR merging. Pushed #19915.

lukemulks · 2023-09-14T20:58:05Z

Hi @petemill @tmancey @simonhong do we have an eta for when this is likely to be committed? I'd heard we were aiming for the release this week, but it looks like this is still in progress. Asking so I can give sales team an update on this too. Thanks.

components/ntp_background_images/browser/view_counter_model.cc

components/ntp_background_images/browser/view_counter_model_unittest.cc

lukemulks · 2023-09-19T00:59:06Z

Hi all, apologies for the bump, but wanted to follow-up on my prior note above from last week. Thanks @tmancey @petemill @simonhong

simonhong · 2023-09-19T01:52:08Z

I left some trivial comments but this PR looks good to me except that.

lukemulks · 2023-09-21T15:44:35Z

@tmancey @petemill given @simonhong comment above, can we please prioritize getting this committed and merged? Team is pretty eager to get a read on potential volume increases with some large customers deploying campaigns over the next two weeks. Thanks!

petemill · 2023-09-25T04:51:15Z

Sorry @lukemulks, I missed the countless notifications somehow. And as far as I can tell, we decided to not do some of this:

Change post-install first-run count from 2nd to 1st.
- we should not do this anymore
Change regular interval count from every 4 NTP view to every 3.
- We should still do this one, though we can do it without this PR via griffin already
Reset the counter every 24hr or browser restart to the post-install first-run count.
- If we aren't doing (1), then maybe we shouldn't do this one anymore. This would help for the users who are at 2 more NTP views to go before the NTT will show, as it will reset them to 1 more NTP views to go. However, for the users who are at 0 more NTP view to go before the NTT will show, we would be adding a delay of 1 extra NTP view which they wouldn't have had previously.

The argument for keeping (3) despite also keeping initial NTT count to the second NTP view is that it ensures that users: a) see at least one NTT on the second NTP each day and b) have their first NTP of the day be not a NTT. So the improvement over only implementing (2) is that users who only open 2 NTP a day will always see 1 NTT. If we don't implement (3), then those users will see 0 NTT.

Since this is a test to see the effect, I'm inclined to suggest we keep (3). Just want to make sure we're clear where we're landing as there were a few opinions.

EDIT
I mitigated the issue somewhat by using the minimum of the initial count or the current count so that when new NTT data is received or the 24hr timer expires, we don't lose the NTT view if the user was already scheduled to see a NTT on the next NTP.
This PR is now ready IMO.

… thereafter

…of initial count

simonhong

++

mkarolin

chromium_src++

lukemulks · 2023-09-25T16:39:34Z

Thanks @petemill agree on keeping item 3 for purposes of testing. Thanks!

lukemulks · 2023-09-25T16:46:06Z

@rebron @tackley per ads gtm call - ntt test changes ^^

bridiver

chromium_src changes ok

lukemulks · 2023-10-01T15:58:26Z

Thank you all for the team effort getting this through. Much appreciated 🫡

petemill requested review from tmancey and simonhong August 22, 2023 22:42

petemill self-assigned this Aug 22, 2023

tmancey reviewed Aug 23, 2023

View reviewed changes

simonhong reviewed Aug 24, 2023

View reviewed changes

tmancey reviewed Aug 24, 2023

View reviewed changes

simonhong reviewed Sep 15, 2023

View reviewed changes

components/ntp_background_images/browser/view_counter_model.cc Outdated Show resolved Hide resolved

components/ntp_background_images/browser/view_counter_model_unittest.cc Outdated Show resolved Hide resolved

NTP SI count defaults updated to 1st view after install and every 3rd…

5f50755

… thereafter

petemill force-pushed the ntp-si-initial-count-update branch from 64d8f1b to f90bc02 Compare September 25, 2023 04:59

petemill added 3 commits September 24, 2023 23:22

NTP SI uses initial count after every browser restart and SI data update

124b6b8

NTP SI distance to SI view gets reset to initial count every 1 day

c69a262

View counter resets timer on Reset and uses minimum of current count …

649c917

…of initial count

petemill force-pushed the ntp-si-initial-count-update branch from f90bc02 to 649c917 Compare September 25, 2023 06:22

Migrate old ViewCounterService pref: brave.count_to_branded_wallpaper

22c14c5

petemill requested a review from a team as a code owner September 25, 2023 06:56

simonhong approved these changes Sep 25, 2023

View reviewed changes

mkarolin approved these changes Sep 25, 2023

View reviewed changes

bridiver approved these changes Sep 26, 2023

View reviewed changes

petemill merged commit 3c5e87d into master Sep 27, 2023
6 checks passed

petemill deleted the ntp-si-initial-count-update branch September 27, 2023 05:38

github-actions bot added this to the 1.60.x - Nightly milestone Sep 27, 2023

This was referenced Oct 11, 2023

Additional NTP is needed to show NTP SI in opt-in state when initial_count_to_branded_wallpaper is zero brave/brave-browser#31951

Closed

Make NTP SI kCountsResetTimeDelay configurable via griffin/feature brave/brave-browser#33571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petemill commented Aug 22, 2023 •

edited

Loading

tmancey Aug 23, 2023

petemill Aug 23, 2023

tmancey Aug 23, 2023 •

edited

Loading

petemill Aug 23, 2023

simonhong Aug 24, 2023

simonhong Aug 24, 2023

petemill Sep 25, 2023

tmancey Aug 24, 2023

simonhong commented Aug 28, 2023

simonhong commented Aug 29, 2023

lukemulks commented Sep 14, 2023

lukemulks commented Sep 19, 2023

simonhong commented Sep 19, 2023

lukemulks commented Sep 21, 2023

petemill commented Sep 25, 2023 •

edited

Loading

simonhong left a comment

mkarolin left a comment

lukemulks commented Sep 25, 2023

lukemulks commented Sep 25, 2023

bridiver left a comment

lukemulks commented Oct 1, 2023

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

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

Conversation

petemill commented Aug 22, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

tmancey Aug 23, 2023

Choose a reason for hiding this comment

petemill Aug 23, 2023

Choose a reason for hiding this comment

tmancey Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

petemill Aug 23, 2023

Choose a reason for hiding this comment

simonhong Aug 24, 2023

Choose a reason for hiding this comment

simonhong Aug 24, 2023

Choose a reason for hiding this comment

petemill Sep 25, 2023

Choose a reason for hiding this comment

tmancey Aug 24, 2023

Choose a reason for hiding this comment

simonhong commented Aug 28, 2023

simonhong commented Aug 29, 2023

lukemulks commented Sep 14, 2023

lukemulks commented Sep 19, 2023

simonhong commented Sep 19, 2023

lukemulks commented Sep 21, 2023

petemill commented Sep 25, 2023 • edited Loading

simonhong left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

lukemulks commented Sep 25, 2023

lukemulks commented Sep 25, 2023

bridiver left a comment

Choose a reason for hiding this comment

lukemulks commented Oct 1, 2023

petemill commented Aug 22, 2023 •

edited

Loading

tmancey Aug 23, 2023 •

edited

Loading

petemill commented Sep 25, 2023 •

edited

Loading