Rewards check includes #25685

bridiver · 2024-09-21T22:35:23Z

Resolves brave/brave-browser#10618

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 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

zenparsing

This is great - thanks!

zenparsing · 2024-09-22T15:45:53Z

browser/brave_rewards/rewards_tab_helper.cc

 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #endif

 namespace brave_rewards {

+#if !BUILDFLAG(IS_ANDROID)
+class BraveBrowserListObserver : public BrowserListObserver {


Up to you, but since this is only used within the tab helper, it could be a nested class or be given a more specific name.

I wasn't really sure what to call it, but I'm open to suggestions

goodov · 2024-09-23T07:34:11Z

browser/brave_rewards/BUILD.gn

+source_set("brave_rewards") {
+  sources = [
+    "//brave/browser/brave_rewards/rewards_service_factory.h",
+    "//brave/browser/brave_rewards/rewards_tab_helper.h",


nit: consider fixing these paths to be local in all new targets you introduce in this PR:

"rewards_service_factory.h", "rewards_tab_helper.h",

oops, yep, copy-pasted the list and forgot to remove the full paths

goodov · 2024-09-23T07:37:36Z

browser/brave_rewards/rewards_tab_helper.cc

+  }
+
+ private:
+  raw_ptr<RewardsTabHelper> tab_helper_;  // Not owned.


const all constructor-initialized pointers to trigger compile errors if initialization is missing.

goodov · 2024-09-23T07:46:12Z

browser/ui/BUILD.gn

+source_set("brave_rewards_source") {
+  sources = [
+    "webui/brave_rewards_source.cc",
+    "webui/brave_rewards_source.h",


there's ui/webui/brave_rewards dir. Would be nice to move this into it.

trying to avoid any changes that aren't necessary here because it already touches a lot of stuff

github-actions · 2024-09-23T15:58:37Z

[puLL-Merge] - brave/brave-core@25685

Here's my review of the pull request:

Description

This PR refactors the Brave Rewards service implementation to reduce dependencies and improve modularity. The main changes include:

Decoupling RewardsServiceImpl from Profile, using more specific dependencies instead.
Restructuring the build files to separate the Brave Rewards implementation from its interfaces.
Cleaning up include statements and removing unused dependencies.
Adjusting tests to work with the new RewardsServiceImpl constructor.

Changes

Changes

browser/brave_rewards/BUILD.gn

Added new source sets for brave_rewards and brave_rewards_impl.
Moved sources from sources.gni into these new source sets.
Added conditional compilation for Greaselion and extensions support.

browser/brave_rewards/rewards_service_factory.cc

Updated RewardsServiceImpl constructor call to use specific services instead of Profile.
Added FaviconService and BitmapFetcherService as dependencies.

browser/brave_rewards/rewards_tab_helper.cc and .h

Introduced BraveBrowserListObserver to handle browser list observations.

browser/greaselion/BUILD.gn

Created a new BUILD.gn file for the Greaselion component.

browser/sources.gni

Removed references to brave_rewards_sources and greaselion_sources.
Added reference to new Brave Rewards Android sources.

browser/ui/BUILD.gn

Created a new source set for brave_rewards_source.

components/brave_rewards/browser/BUILD.gn

Updated dependencies and removed unnecessary public_deps.

components/brave_rewards/browser/rewards_service_impl.cc and .h

Refactored constructor to take specific services instead of Profile.
Replaced profile_ member with individual service members.
Updated methods to use the new service members instead of accessing through profile_.

components/brave_rewards/browser/rewards_service_impl_unittest.cc and rewards_service_impl_jp_unittest.cc

Updated test setup to use the new RewardsServiceImpl constructor.

Possible Issues

The refactoring might have introduced subtle changes in behavior due to the different way services are now accessed. Thorough testing of all Brave Rewards functionality is recommended.

Security Hotspots

No significant security issues were identified in this change.

Overall, this refactoring appears to be a positive change that improves the modularity and testability of the Brave Rewards service. It reduces coupling with the Profile class and makes dependencies more explicit, which should make the code easier to maintain and extend in the future.

fmarier

browser/tor/test/BUILD.gn change 👍

bridiver · 2024-10-09T01:13:52Z

components/brave_rewards/browser/BUILD.gn

    "//brave/components/resources",
    "//brave/components/services/bat_rewards/public/interfaces",
+    "//brave/components/time_period_storage",
+    "//chrome/browser/bitmap_fetcher",  # bad


oops, I totally missed this before I merged it. Need to fix this one

bridiver requested review from a team as code owners September 21, 2024 22:35

github-actions bot added feature/web3/wallet feature/web3/wallet/core labels Sep 21, 2024

github-actions bot assigned bridiver Sep 21, 2024

github-actions bot added the puLL-Merge label Sep 21, 2024

bridiver changed the base branch from master to rewards_api_deps September 21, 2024 22:36

Base automatically changed from rewards_api_deps to master September 22, 2024 13:16

remove check_includes = false for brave_rewards

c8ef420

bridiver force-pushed the rewards_check_includes branch from 590e715 to c8ef420 Compare September 22, 2024 13:39

bridiver added 7 commits September 22, 2024 07:57

fix buildflag guard

dbde163

move brave_rewards android sources to sources.gni

5b9451a

fix copyright

65b9983

path fixes

5280aa9

fix buildflag guards

a955b51

android fixes

d19a800

fix headers

78956df

zenparsing approved these changes Sep 22, 2024

View reviewed changes

add missing android_util header

8ae9c76

goodov reviewed Sep 23, 2024

View reviewed changes

fix include path

125d86d

remove full paths

b86f80b

fmarier approved these changes Sep 23, 2024

View reviewed changes

const

1c83d10

darkdh approved these changes Sep 23, 2024

View reviewed changes

bridiver merged commit 2e85065 into master Sep 23, 2024
17 checks passed

bridiver deleted the rewards_check_includes branch September 23, 2024 21:07

github-actions bot added this to the 1.72.x - Nightly milestone Sep 23, 2024

bridiver commented Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards check includes #25685

Rewards check includes #25685

bridiver commented Sep 21, 2024 •

edited

Loading

zenparsing left a comment

zenparsing Sep 22, 2024

bridiver Sep 22, 2024

goodov Sep 23, 2024

bridiver Sep 23, 2024

goodov Sep 23, 2024

goodov Sep 23, 2024

bridiver Sep 23, 2024

github-actions bot commented Sep 23, 2024

Changes

browser/brave_rewards/BUILD.gn

browser/brave_rewards/rewards_service_factory.cc

browser/brave_rewards/rewards_tab_helper.cc and .h

browser/greaselion/BUILD.gn

browser/sources.gni

browser/ui/BUILD.gn

components/brave_rewards/browser/BUILD.gn

components/brave_rewards/browser/rewards_service_impl.cc and .h

components/brave_rewards/browser/rewards_service_impl_unittest.cc and rewards_service_impl_jp_unittest.cc

fmarier left a comment

bridiver Oct 9, 2024

Rewards check includes #25685

Rewards check includes #25685

Conversation

bridiver commented Sep 21, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

zenparsing left a 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

github-actions bot commented Sep 23, 2024

Description

Changes

browser/brave_rewards/BUILD.gn

browser/brave_rewards/rewards_service_factory.cc

browser/brave_rewards/rewards_tab_helper.cc and .h

browser/greaselion/BUILD.gn

browser/sources.gni

browser/ui/BUILD.gn

components/brave_rewards/browser/BUILD.gn

components/brave_rewards/browser/rewards_service_impl.cc and .h

components/brave_rewards/browser/rewards_service_impl_unittest.cc and rewards_service_impl_jp_unittest.cc

Possible Issues

Security Hotspots

fmarier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Sep 21, 2024 •

edited

Loading