Explicitly set the default view for shield settings #4860
Conversation
| kShieldsAdvancedViewEnabled)); | ||
| // verify that pref was set (and is not default) | ||
| const PrefService::Preference* pref = |
There was a problem hiding this comment.
updated test to verify value IS SET 👍
bsclifton
added
CI/skip-android
|
CI passed on all platforms, Windows ran into an issue during |
|
@bsclifton Thanks for the hustle!! :) |
Defaults to: - advanced for existing users - simple for new users Changing the default was not "locking in" the setting. This change locks in that default. Fixes brave/brave-browser#8533
bsclifton
force-pushed
the
bsc-fix-default-shields
branch
from
7e58f3d
to
2ae4fef
Compare
bsclifton
removed
CI/skip-android
| EXPECT_TRUE( | ||
| browser()->profile()->GetPrefs()->GetBoolean( | ||
| kShieldsAdvancedViewEnabled)); | ||
| // verify that pref was set (and is not default) |
There was a problem hiding this comment.
I think this test is redundant with above one? what is the purpose of this test?
Each browser tests uses fresh profile.
I assume you want spanning browser test? (https://www.chromium.org/developers/testing/browser-tests)
There was a problem hiding this comment.
Goal for this test is to have a non-first-run profile and verify that value does get set and that it's what we expect (advanced) 😄
Versus the one above, which is having an empty first-run profile and then verifying it gets simple view
There was a problem hiding this comment.
Oh, I didn't know profile that used for this test is non-first-run profile :)
I thought all browser tests uses fresh test profile.
bsclifton
force-pushed
the
bsc-fix-default-shields
branch
from
2ae4fef
to
42f95d4
Compare
|
CI looks good! Only had lint/deps failures which are being addressed in Master too. Good to merge 🎉 |
This was intended to lock in advanced view for existing users and has done its job :) It's now obsolete Fixes brave/brave-browser#12104 This contains a manual revert of #3154 and #4860
Defaults to:
Changing the default was not "locking in" the setting. This change
locks in that default.
Fixes brave/brave-browser#8533
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
See brave/brave-browser#8533
Reviewer Checklist:
After-merge Checklist:
changes has landed on.