-
Notifications
You must be signed in to change notification settings - Fork 887
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 state-level ad targeting shown for non-US region #11216
Fix state-level ad targeting shown for non-US region #11216
Conversation
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.
LGTM
ecd4452
to
f7f146a
Compare
@@ -546,6 +546,23 @@ bool AdsServiceImpl::ShouldStart() const { | |||
return IsEnabled() || IsBraveNewsEnabled(); | |||
} | |||
|
|||
base::DictionaryValue AdsServiceImpl::GetSettingsAsValue() { |
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.
It looks like the motivation for moving this JSON-construction code into the service layer is to avoid sharing it between the rewards page and the rewards extension API, and that the Ads service does JSON-construction already in GetInlineContentAd
.
It seems to me that the service layer shouldn't be concerned with how the front-end might need data packaged up, but we currently don't have a good alternative for where to put this code. Options are:
- Leave the code duplicated for now, since we expect to deprecate the rewards page in the future.
- Move this code to the rewards extension API and have the rewards page use that (I'm pretty sure that
chrome.braveRewards
is already used there. - Introduce some new class (service?) for JSON-ifying data for rewards UIs.
My personal preference would be for 1 or 2. I'd probably shy away from adding the code to the Ads service since the Ads service shouldn't need to know how the data is used on the settings page or rewards page.
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.
Makes total sense, I'll just copy the function for now.
1dac49a
to
2548435
Compare
2548435
to
2f2d9a0
Compare
@emerick I got a DCHECK when I open Settings page:
Seems like
|
Thanks for catching and reporting this @goodov! I entered brave/brave-browser#19802 to track this. |
…-level-targeting Fix state-level ad targeting shown for non-US region
Resolves brave/brave-browser#19589
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
,npm run lint
,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 STR in brave/brave-browser#19589