-
Notifications
You must be signed in to change notification settings - Fork 920
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
Add support for default feature state overrides using static map #10878
Conversation
4707962
to
3e622d0
Compare
@@ -210,76 +188,13 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) { | |||
command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL, | |||
kVariationsServerURL.c_str()); | |||
|
|||
// Enabled features. |
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.
JFTR: I've checked all enabled features one by one to confirm all were properly migrated and that seems to be the case 👍
|
||
// Disabled features. |
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.
JFTR: I've checked all disabled features one by one to confirm all were properly migrated and that seems to be the case 👍
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!
chromium_src/components/reading_list/features/reading_list_switches.cc
Outdated
Show resolved
Hide resolved
Sorry reviewers, I decided to went with a single macro which should be used to bulk-override all Feel free to review once more. Sorry again :) I added this change as a new commit on top. |
constexpr Feature kTestConstexprDisabledButOverridenFeature{ | ||
"TestConstexprDisabledButOverridenFeature", FEATURE_ENABLED_BY_DEFAULT}; | ||
|
||
OVERRIDE_FEATURE_DEFAULT_STATES({{ |
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.
the way this macro should be used is intentional. If we put {
or {{
inside the macro, then clang-format wouldn't be able to properly format lines which are #if defined(...)
. Example:
OVERRIDE_FEATURE_DEFAULT_STATES({
{kReadLater, base::FEATURE_DISABLED_BY_DEFAULT},
#if defined(OS_ANDROID)
{kReadLater2, base::FEATURE_DISABLED_BY_DEFAULT},
#endif
{kReadLater3, base::FEATURE_DISABLED_BY_DEFAULT},
});
f8e96a9
to
a035be5
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 (also liked the latest changes 👍 )
chromium_src/base/feature_list.cc
Outdated
|
||
namespace internal { | ||
|
||
FeatureDefaultStateOverride::FeatureDefaultStateOverride( |
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.
I am probably missing something obvious here, but why do this via a class and construct all those global objects instead of using a global function to populate the map?
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.
The question is at what point you have to populate the map and how to make sure it will happen in all processes. The global construction approach make it so in each process (utility, renderer, etc), the map is populated at the very start regardless of what Chromium/Brave delegate is called or not.
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.
Oh I see, yep, that totally makes sense. 👍
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.
++
a035be5
to
a1b29ca
Compare
Resolves brave/brave-browser#18829
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: