-
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
Brave now builds if brave_rewards_enabled is false #2462
Conversation
|
||
#include "brave/components/brave_extension/grit/brave_extension_generated_map.h" | ||
#include "brave/components/brave_extension/grit/brave_extension_resources_map.h" | ||
#if BUILDFLAG(BRAVE_REWARDS_ENABLED) |
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.
nit: There is an implicit convention in chromium about header inclusion in guard.
Usually they are positioned at last like below.
#include "ui/views/widget/tooltip_manager.h"
#include "ui/views/widget/widget.h"
#if defined(OS_WIN)
#include "base/win/scoped_gdi_object.h"
#include "ui/native_theme/native_theme_win.h"
#endif
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.
Done.
@@ -7,7 +7,10 @@ | |||
#include "brave/browser/ui/views/location_bar/brave_location_bar_view.h" | |||
#include "brave/common/extensions/extension_constants.h" | |||
#include "brave/common/pref_names.h" | |||
#include "brave/components/brave_rewards/browser/buildflags/buildflags.h" | |||
#if BUILDFLAG(BRAVE_REWARDS_ENABLED) |
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.
ditto
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.
Done.
f366e89
to
51c4899
Compare
51c4899
to
a5b0fae
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.
LGTM
…prints Note current signing key fingerprints
FYI, this is no longer working :) |
@sebionofrei can you please raise a new issue, thank you |
Fixes brave/brave-browser#4069
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
components/brave_rewards/browser/buildflags/buildflags.gni
fromto
npm run build
and make sure build is successfulnpm start
and make sure Brave runs with Rewards disabled without errorsnpm run test -- brave_unit_tests && npm run test -- brave_browser_tests
and make sure all tests passbuildflags.gni
back to original state and repeat steps 2-4 with step 3 ensuring that Rewards functions correctlyReviewer Checklist:
After-merge Checklist:
changes has landed on.