Add Rewards settings to brave://settings #10610

emerick · 2021-10-20T13:31:30Z

Resolves brave/brave-browser#18158

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
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, npm run lint, 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:

Screenshot:

Test Plan:

Clean profile
Visit brave://settings
Confirm that Rewards section is empty except for message about enabling Rewards
Click the link in the above message
Verify that Rewards panel opens
Enable Rewards
Verify that brave://settings page updates with Rewards settings and that settings reflect current Rewards state
Verify that changing settings in brave://settings reflects the same changes in brave://rewards and vice versa
Verify that enabling/disabling main options enables/disables sub-options
Verify that preference changes persist across browser restarts
Verify that dropdowns with "Default" selected work as expected (i.e., use the default value for that preference)
Verify that Ads state dropdown works as expected, including Auto and Disabled settings

zenparsing · 2021-10-21T15:28:41Z

browser/ui/webui/brave_rewards_page_ui.cc

+}
+
+void RewardsDOMHandler::OnPreferenceChanged(const std::string& key) {
+  AllowJavascript();


Without changing anything else, we should probably no-op if we don't currently "allow" JS. Separately, I think the desired pattern is to only have observers registered from the JavascriptAllowed method.

Ah yes, fixed.

zenparsing · 2021-10-21T15:32:28Z

browser/ui/webui/brave_rewards_page_ui.cc

+void RewardsDOMHandler::OnPreferenceChanged(const std::string& key) {
+  AllowJavascript();
+  GetEnabledInlineTippingPlatforms(base::Value::ConstListView());
+  GetAutoContributionAmount(base::Value::ConstListView());


By calling these message handlers directly, we're more-or-less acting on behalf of the front-end code. I don't have a strong opinion on that, but consider the tradeoffs of this versus the alternative that has the front-end listen for a generic "pref change" message and trigger these calls itself.

That's a really good point. Updated as you suggested.

zenparsing · 2021-10-21T15:57:31Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.html

+    pref="{{prefs.brave.rewards.inline_tip.github}}">
+  </settings-toggle-button>
+</template>
+<template is="dom-if" if="[[!isRewardsEnabled_(prefs.brave.rewards.enabled.value, prefs.brave.brave_ads.enabled.value)]]" restamp>


Is it possible to just use prefs.brave.rewards.enabled?

Yes, meant to fix this - thank you!

zenparsing · 2021-10-21T15:59:29Z

browser/resources/settings/brave_rewards_page/brave_rewards_browser_proxy.js

+export class BraveRewardsBrowserProxyImpl {
+  /** @override */
+  isRewardsEnabled () {
+    return new Promise((resolve) => chrome.braveRewards.shouldShowOnboarding(


FYI: there's also a chrome.braveRewards.getRewardsEnabled function for just getting that pref (if you even need that from the settings page).

I would like to see us deprecate shouldShowOnboarding (and certainly not use it here), because its usage is not well-defined. It looks like for this use case we should just be able to use the rewards.enabled pref directly (since it just controls whether the "params" are downloaded)?

Does relying on rewards.enabled introduce any backwards-compatibility problems? I'm sort of forgetting how these all changed over time. Totally agree that relying on the onboarding key is no good, so I did make this change.

petemill · 2021-10-25T21:43:46Z

browser/resources/settings/brave_rewards_page/brave_rewards_browser_proxy.html

@@ -0,0 +1,2 @@
+<link rel="href" src="chrome://resources/html/cr.html">


This file isn't neccessary

Thanks, removed.

petemill · 2021-10-25T21:48:18Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.html

+    disabled="[[!prefs.brave.rewards.ac.enabled.value]]">
+  </settings-toggle-button>
+  <div class="settings-box continuation">
+    <p>$i18n{braveRewardsAutoContributeDescLabel} <a href="https://brave.com/terms-of-use/" target="_blank" rel="noopener">Terms of Service</a> and <a href="https://brave.com/privacy/browser/" target="_blank" rel="noopener">Privacy Policy</a>.</p></p>


Should the end of the sentence " Terms of Service and Privacy Policy." not be translated too? This might come out in a weird order in some languages.

Yes, good catch - fixed.

petemill · 2021-10-25T22:41:10Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.js

+
+  ready() {
+    super.ready()
+    this.openRewardsPanel_ = this.openRewardsPanel_.bind(this)


Perhaps you can use arrow function instead? openRewardsPanel_ = () => {

petemill · 2021-10-25T22:42:11Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.js

+  ready() {
+    super.ready()
+    this.openRewardsPanel_ = this.openRewardsPanel_.bind(this)
+    this.maxAdsToDisplay_ = [


Thses 4 variables can be instance variables declared outside the ready() constructor? Or even global static variables?

petemill · 2021-10-25T22:43:18Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.js

+  }
+
+  shouldAllowAdsSubdivisionTargeting_() {
+    return navigator.language === 'en-US'


Should this come from the backend API? Is there a possibility of navigator.language being different than the backend locale logic?

Good point, added a function to the backend API.

tmancey · 2021-10-27T18:19:41Z

browser/ui/webui/brave_rewards_page_ui.cc

@@ -285,6 +287,9 @@ class RewardsDOMHandler
  // AdsServiceObserver implementation
  void OnAdRewardsChanged() override;

+  void InitPrefChangeRegistrar();
+  void OnPreferenceChanged(const std::string& key);


nit: OnPrefChanged to be consistent with Chromium

tmancey · 2021-10-27T18:21:19Z

browser/ui/webui/brave_rewards_page_ui.cc

+                          base::Unretained(this)));
+}
+
+void RewardsDOMHandler::OnPreferenceChanged(const std::string& key) {


nit: rename key to path to match components/prefs/pref_change_registrar.h

Sure, fixed.

tmancey

LGTM

tmancey · 2021-10-27T18:27:44Z

Not related directly to this PR. However, when we add subdivision targeting for Canada (brave/brave-browser#16682) "State-level ad targeting" will no longer make any sense for Canadian users (or other users when this feature is rolled out to other countres). We should look at rewording to "Subdivision-level ad targeting" as defined by the following https://en.wikipedia.org/wiki/ISO_3166-2 which is the universal naming convention. cc @Miyayes @jsecretan

Miyayes · 2021-10-27T18:32:38Z

Makes sense from standards perspective (since Canada has provinces, Japan has prefectures, etc.), but from user perspective, I think the word "subdivision-level" could be quite confusing. Users would quickly understand once they see the list of provinces, of course, but I have never heard it used before. @rmcfadden3 might have some useful suggestions for generic wording.

Edit: We may just go with translations for each region.

tmancey · 2021-10-27T18:35:30Z

@Miyayes I agree. But in the United Kingdom we call them Counties, so if this feature was ever rolled out to the UK then states would be even more confusing in my opinion

szilardszaloki · 2021-10-27T20:58:23Z

browser/extensions/api/brave_rewards_api.cc

+BraveRewardsGetLocaleFunction::~BraveRewardsGetLocaleFunction() {}
+
+ExtensionFunction::ResponseAction BraveRewardsGetLocaleFunction::Run() {
+  const std::string locale =


Nit:

std::string locale = brave_l10n::LocaleHelper::GetInstance()->GetLocale(); return RespondNow(OneArgument(base::Value(std::move(locale))));

Thanks, fixed.

szilardszaloki

LGTM

goodov · 2021-10-28T09:28:11Z

browser/resources/settings/brave_rewards_page/brave_rewards_page.html

+    </div>
+  </template>
+  <div class="settings-box continuation">
+    <p>$i18n{braveRewardsStateLevelAdTargetingDescLabel} <a href="https://support.brave.com/hc/en-us/articles/360026361072-Brave-Ads-FAQ" target="_blank" rel="noopener">Learn more</a></p>


"Learn more" should be a part of a localizable string, or better - replace it with $i18n{learnMore}.

Good catch, fixed.

goodov

++ chromium_src

emerick self-assigned this Oct 20, 2021

emerick force-pushed the rewards-settings-in-brave-settings branch from ceb35b8 to 68d3e9a Compare October 20, 2021 13:44

emerick changed the title ~~Add Rewards settings to brave://settings~~ WIP: Add Rewards settings to brave://settings Oct 20, 2021

emerick requested review from zenparsing and tmancey October 20, 2021 16:07

emerick force-pushed the rewards-settings-in-brave-settings branch 3 times, most recently from c8f41e1 to c293127 Compare October 21, 2021 15:44

zenparsing reviewed Oct 21, 2021

View reviewed changes

emerick force-pushed the rewards-settings-in-brave-settings branch from c293127 to a547199 Compare October 21, 2021 20:55

emerick requested a review from petemill October 22, 2021 16:11

petemill reviewed Oct 25, 2021

View reviewed changes

emerick force-pushed the rewards-settings-in-brave-settings branch 5 times, most recently from 64ef4bd to 70f1046 Compare October 27, 2021 10:41

emerick marked this pull request as ready for review October 27, 2021 10:52

emerick requested review from a team as code owners October 27, 2021 10:52

emerick requested a review from jumde October 27, 2021 10:52

emerick force-pushed the rewards-settings-in-brave-settings branch from 70f1046 to af0f90a Compare October 27, 2021 10:53

emerick changed the title ~~WIP: Add Rewards settings to brave://settings~~ Add Rewards settings to brave://settings Oct 27, 2021

emerick force-pushed the rewards-settings-in-brave-settings branch from af0f90a to a823c45 Compare October 27, 2021 13:26

tmancey reviewed Oct 27, 2021

View reviewed changes

tmancey approved these changes Oct 27, 2021

View reviewed changes

emerick removed the request for review from jumde October 27, 2021 18:37

emerick force-pushed the rewards-settings-in-brave-settings branch 2 times, most recently from b4fa4ea to 99097c6 Compare October 27, 2021 18:47

szilardszaloki reviewed Oct 27, 2021

View reviewed changes

emerick force-pushed the rewards-settings-in-brave-settings branch 2 times, most recently from 5bad17b to 84588e3 Compare October 27, 2021 21:09

emerick requested a review from zenparsing October 27, 2021 21:16

szilardszaloki approved these changes Oct 27, 2021

View reviewed changes

goodov reviewed Oct 28, 2021

View reviewed changes

Add Rewards settings to brave://settings

4532a32

emerick force-pushed the rewards-settings-in-brave-settings branch from 84588e3 to 4532a32 Compare October 28, 2021 12:01

emerick requested a review from goodov October 28, 2021 12:02

goodov approved these changes Oct 28, 2021

View reviewed changes

emerick merged commit 5ba15e0 into master Oct 28, 2021

emerick deleted the rewards-settings-in-brave-settings branch October 28, 2021 14:34

emerick added this to the 1.33.x - Nightly milestone Oct 28, 2021

GeetaSarvadnya mentioned this pull request Nov 16, 2021

Implement Rewards settings section inside brave://settings brave/brave-browser#18158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rewards settings to brave://settings #10610

Add Rewards settings to brave://settings #10610

emerick commented Oct 20, 2021 •

edited

Loading

zenparsing Oct 21, 2021

emerick Oct 21, 2021

zenparsing Oct 21, 2021

emerick Oct 21, 2021

zenparsing Oct 21, 2021

emerick Oct 21, 2021

zenparsing Oct 21, 2021

emerick Oct 21, 2021

petemill Oct 25, 2021

emerick Oct 26, 2021

petemill Oct 25, 2021

emerick Oct 26, 2021

petemill Oct 25, 2021

emerick Oct 26, 2021

petemill Oct 25, 2021

emerick Oct 26, 2021

petemill Oct 25, 2021

emerick Oct 26, 2021

tmancey Oct 27, 2021

emerick Oct 27, 2021

tmancey Oct 27, 2021

emerick Oct 27, 2021

tmancey left a comment

tmancey commented Oct 27, 2021 •

edited

Loading

Miyayes commented Oct 27, 2021 •

edited

Loading

tmancey commented Oct 27, 2021

szilardszaloki Oct 27, 2021

emerick Oct 27, 2021

szilardszaloki left a comment

goodov Oct 28, 2021 •

edited

Loading

emerick Oct 28, 2021

goodov left a comment

		@@ -0,0 +1,2 @@
		<link rel="href" src="chrome://resources/html/cr.html">

Add Rewards settings to brave://settings #10610

Add Rewards settings to brave://settings #10610

Conversation

emerick commented Oct 20, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Screenshot:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey left a comment

Choose a reason for hiding this comment

tmancey commented Oct 27, 2021 • edited Loading

Miyayes commented Oct 27, 2021 • edited Loading

tmancey commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szilardszaloki left a comment

Choose a reason for hiding this comment

goodov Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov left a comment

Choose a reason for hiding this comment

emerick commented Oct 20, 2021 •

edited

Loading

tmancey commented Oct 27, 2021 •

edited

Loading

Miyayes commented Oct 27, 2021 •

edited

Loading

goodov Oct 28, 2021 •

edited

Loading