-
Notifications
You must be signed in to change notification settings - Fork 868
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
Fix news P3A enabled metric #18701
Fix news P3A enabled metric #18701
Conversation
d664e03
to
69fc60f
Compare
@@ -57,6 +58,9 @@ namespace { | |||
// The favicon size we desire. The favicons are rendered at 24x24 pixels but | |||
// they look quite a bit nicer if we get a 48x48 pixel icon and downscale it. | |||
constexpr uint32_t kDesiredFaviconSizePixels = 48; | |||
// Since we have two boolean prefs for the News enabled status, a delay | |||
// will be used so that we only report the histogram once for both pref updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem with reporting the histogram twice - won't it just report only the most recent value for this metric?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite, it seems that when a user presses "no thanks" on Android, both prefs will be enabled in the first call, and then the "show today" pref will be disabled in the second call. if we don't have the timer, the first call will set the "was enabled" pref to true which will result in the "is enabled" metric being reported as 0 in the second call, which we don't want. the timer is here to make sure all edge cases are covered and increases robustness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a bug from Android. We should locate that part @deeppandya @tapanmodh ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petemill sure. I'll check it
69fc60f
to
2ba6fbb
Compare
Resolves brave/brave-browser#30709
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Follow plan in #18375