-
Notifications
You must be signed in to change notification settings - Fork 888
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
Disable Greaselion if Rewards is off #25535
Disable Greaselion if Rewards is off #25535
Conversation
@@ -270,6 +270,9 @@ GreaselionServiceImpl::GetExtensionIdsForTesting() { | |||
} | |||
|
|||
void GreaselionServiceImpl::UpdateInstalledExtensions() { | |||
if (!delegate_->IsEnabled()) { | |||
return; |
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.
Will this unload currently installed Greaselion extensions if it's disabled? If not, should it? I'm wondering if this check could go in CreateAndInstallExtensions
. @emerick WDYT?
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.
Nope, it doesn't just to simplify the code.
IMHO it's okay to have the extensions loaded if the users changed Rewards state, since it's a rare case.
The goal is to cover the usual scenarios to improve perf.
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 sounds fine. In that case, the intended behavior here is that GL extensions will not be added or updated if Rewards is disabled. They will remain as-is if Rewards is disabled via reset.
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.
Makes sense to me.
#include "brave/components/greaselion/browser/greaselion_service.h" | ||
#include "brave/components/greaselion/browser/greaselion_service_impl.h" | ||
#include "chrome/browser/extensions/extension_service.h" | ||
#include "chrome/browser/profiles/profile.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.
Can we check if these need deps?
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.
thanks, updated sources.gni
.
Frankly, I don't full understand that approach with "components" in brave/browser/ until chrome does the same.
If fact, we push all the deps into a one list and non on them can be build separately.
03bfc8d
to
e443e96
Compare
* Disable Greaselion if Rewards is off * update deps
Resolves brave/brave-browser#40872
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:
brave/brave-browser#39610 (comment)