-
Notifications
You must be signed in to change notification settings - Fork 905
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
Reconfigure Rewards component folder structure #27270
base: master
Are you sure you want to change the base?
Conversation
be5d45a
to
19f408a
Compare
A Storybook has been deployed to preview UI for the latest push |
19f408a
to
285a0af
Compare
Chromium major version is behind target branch (132.0.6834.83 vs 133.0.6943.27). Please rebase. |
285a0af
to
8f228eb
Compare
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.
Ads LGTM++
.github/CODEOWNERS
Outdated
components/brave_rewards/core/common/signer.h @brave/sec-team | ||
components/brave_rewards/core/common/request_signer.cc @brave/sec-team | ||
components/brave_rewards/core/common/request_signer.h @brave/sec-team | ||
components/brave_rewards/core/internal/common/signer.cc @brave/sec-team |
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.
you still have common
here which is a process directory, but not sure what makes sense here instead
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.
That folder contains utility functions and classes for use by the "engine". The folder could be renamed to "util" or "helpers" or something similar.
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.
either of those options are fine, I just don't want to confuse the use of common
due to its special meaning in chromium
browser/brave_rewards/DEPS
Outdated
@@ -1,5 +1,5 @@ | |||
include_rules = [ | |||
"+brave/components/brave_rewards/core", | |||
"+brave/components/brave_rewards/core/internal", |
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 odd, internal
code shouldn't be accessible outside of the component. This would also allow subdirectories inside core/internal
. Maybe this should have a different directory name or maybe whatever files you need in browser/brave_rewards
should go somewhere else?
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.
In browser/brave_rewards/android/brave_rewards_native_worker.cc
we have the following usage:
brave_rewards::internal::constant::kWalletSolana
This is the only usage of internal
in this folder. Ideally, I think that the constant should be moved out of internal
and (optionally) it should be an enum value instead.
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.
Right, this is why I suggested moving things that are shared out of internal, but it seems like this PR is moving a lot of things into internal? In any case I think it's best to limit this PR to fixing of layered component directory issues and follow-up with other changes to keep the size/scope of this PR down
@@ -1,7 +1,7 @@ | |||
/* Copyright (c) 2020 The Brave Authors. All rights reserved. |
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 is good to move these out of components and I guess you could consider it part of fixing the directory structure, but this PR is a bit large so maybe this should be split out? Also please remove the specific include rules for these in DEPS
@@ -153,7 +153,8 @@ | |||
"test/data/speedreader/", | |||
], | |||
"CheckStableMojomChanges": [ | |||
"components/brave_rewards/common/mojom/rewards_engine\\.mojom", | |||
"components/brave_rewards/common/mojom/.*\\.mojom", | |||
"components/services/bat_rewards/public/interfaces/rewards_engine_factory\\.mojom", |
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.
why does this need to be added?
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 presubmit checks tries to find mojo interfaces that have (accidentally) changed. When mojom files are moved, the check frequently throws an error because it can't find the "old" file. After this PR is merged, we can remove these lines.
@@ -65,7 +65,7 @@ static_library("browser") { | |||
"//url", | |||
] | |||
|
|||
public_deps = [ "//brave/components/brave_rewards/core:headers" ] | |||
public_deps = [ "//brave/components/brave_rewards/core/internal:headers" ] |
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.
similar to the earlier comment making something in internal
a public_dep
here seems odd. Also this particular target didn't have a directory structure issue once the process directories were removed so why move it to internal
?
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.
Background on internal
(currently core
):
This is the cross-platform "Rewards engine" code that currently runs in a utility process on desktop/android, but could potentially run in the browser process in the future. It was originally in third_party/bat-native-ledger
I think.
It seems to me that ideally this folder would be truly internal, and it would only be accessed by iOS or content drivers also in components/brave_rewards
. If we want to move in that direction, then internal
as the folder name makes sense, and I can gradually move us there.
Alternatively, we could name this something like "engine".
@@ -15,8 +15,8 @@ | |||
#include "base/strings/strcat.h" | |||
#include "base/strings/string_split.h" | |||
#include "base/strings/string_util.h" | |||
#include "brave/components/brave_rewards/common/pref_names.h" | |||
#include "brave/components/brave_rewards/core/buildflags.h" | |||
#include "brave/components/brave_rewards/core/internal/buildflags.h" |
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.
buildflags are internal? I think the whole internal
thing is confusing this PR a bit and doesn't seem necessary for fixing the issues with the component directory structure so that should probably also move to a separate PR
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 is odd, I agree (and it was odd even without the internal
name). Can this target be moved into the already existing components/brave_rewards/core/buildflags
target?
65b9e9c
to
a8d083e
Compare
a8d083e
to
73a20a1
Compare
[puLL-Merge] - brave/brave-core@27270 DescriptionThis PR reorganizes the
The main motivation appears to be better code organization and separation of concerns between the browser, common, and core components. ChangesChangesBy filename pattern: Common Files Moved/Renamed (
Browser Files Moved/Renamed (
Test Files Reorganized
sequenceDiagram
participant Browser
participant Common
participant Core
Browser->>Common: Uses shared definitions/interfaces
Browser->>Core: Interacts with rewards engine
Core->>Common: Uses shared types/constants
Security Hotspots
|
Resolves brave/brave-browser#34881
Summary:
Changes:
components/brave_rewards/browser
=>components/brave_rewards/content
components/brave_rewards/browser/test
=>browser/brave_rewards/test
components/brave_rewards/common
=>components/brave_rewards/core
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 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: