Skip to content
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

Default shield advanced view to TRUE for existing users. #3154

Merged
merged 4 commits into from
Aug 15, 2019

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Aug 12, 2019

Fixes brave/brave-browser#5582

Submitter Checklist:

Test Plan:

  1. Have an existing profile (before 0.69.x)
  2. Launch this version
  3. Look in brave://settings and confirm advanced view is chosen
  4. Open brave.com and click shields. Verify advanced view is shown
  5. Close Brave
  6. New profile and launch
  7. Look in brave://settings and confirm simple view is chosen
  8. Open brave.com and click shields. Verify simple view is shown

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton bsclifton added this to the 0.70.x - Nightly milestone Aug 12, 2019
@bsclifton bsclifton requested a review from bridiver as a code owner August 12, 2019 18:31
@bsclifton bsclifton self-assigned this Aug 12, 2019
@bsclifton bsclifton force-pushed the bsc-shields-advanced-default-change branch from e7fad66 to a68dc75 Compare August 12, 2019 18:32
@bsclifton bsclifton removed the request for review from bridiver August 12, 2019 18:33
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that this doesn't affect new additional user profiles that are created. In other words, IsFirstRun() returns true for those profiles too.

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browser test is failing, possibly because of changed expected value

[  FAILED  ] BraveProfilePrefsBrowserTest.MiscBravePrefs, where TypeParam =  and GetParam() =  (1116 ms)
[238/238] BraveProfilePrefsBrowserTest.MiscBravePrefs (1315 ms)
1 test failed:
    BraveProfilePrefsBrowserTest.MiscBravePrefs (../../brave/browser/brave_profile_prefs_browsertest.cc:25)

@bsclifton
Copy link
Member Author

ah- good catch... thanks! 😄 Will address

@bsclifton bsclifton force-pushed the bsc-shields-advanced-default-change branch from a68dc75 to 9165ea2 Compare August 12, 2019 21:07
…g users) and false (first run)

Pulls in some re-usable code from chrome/browser/first_run/first_run_browsertest.cc
@bsclifton
Copy link
Member Author

bsclifton commented Aug 14, 2019

Some lint failures - not sure on the other failures here (some builds worked fine). Will fix the lints soon and push again later tonite

@cezaraugusto
Copy link
Contributor

confirmed manual test works. ++ once tests are passing

cezaraugusto
cezaraugusto previously approved these changes Aug 14, 2019
@bsclifton
Copy link
Member Author

bsclifton commented Aug 14, 2019

Build worked great (all tests + lint passed), except for android. Need to troubleshoot this:
https://staging.ci.brave.com/job/brave-browser-build-pr/job/bsc-shields-advanced-default-change/4/execution/node/425/log/

23:08:30 ld.lld: error: undefined symbol: first_run::IsChromeFirstRun()
23:08:30 >>> referenced by brave_profile_prefs.cc:55 (../../brave/browser/brave_profile_prefs.cc:55)
23:08:30 >>> thinlto-cache/Thin-471adf.tmp.o:(brave::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*))
23:08:30 clang: error: linker command failed with exit code 1 (use -v to see invocation)
23:08:30 ninja: build stopped: subcommand failed.

This fixes error seen compiling for android core
> 23:08:30  ld.lld: error: undefined symbol: first_run::IsChromeFirstRun()
> 23:08:30  >>> referenced by brave_profile_prefs.cc:55 (../../brave/browser/brave_profile_prefs.cc:55)
> 23:08:30  >>>               thinlto-cache/Thin-471adf.tmp.o:(brave::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*))
@bsclifton
Copy link
Member Author

OK this should be G2G now - let's let CI finish 😄

@bsclifton
Copy link
Member Author

@petemill @cezaraugusto can you please re-review? 😄

@bsclifton bsclifton merged commit 590da54 into master Aug 15, 2019
@bsclifton bsclifton deleted the bsc-shields-advanced-default-change branch August 15, 2019 06:45
brave-builds pushed a commit that referenced this pull request Aug 15, 2019
@mkarolin
Copy link
Collaborator

@bsclifton BraveProfilePrefsBrowserTest.AdvancedShieldsExistingUserValue is crashing for me on Windows. CI doesn't run browser tests on Windows :(.

@bsclifton
Copy link
Member Author

bsclifton commented Aug 19, 2019

@mkarolin oh no! Thanks for the heads up (about browser tests not working on Windows and that this fails)

Booting up my VM now to try this out...

If there's not a solution soon, I'll create a PR to revert

@bsclifton
Copy link
Member Author

update: I ran tests on Win64 (in release mode) and it worked great. Might be a DCHECK failure. Will compile debug and re-run

@bsclifton
Copy link
Member Author

bsclifton commented Aug 20, 2019

Error looks like this:

[8784:8388:0819/155955.361:FATAL:thread_restrictions.cc(76)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the ThreadPool, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking (see its documentation for best practices).
g_blocking_disallowed currently set to true by

@bsclifton
Copy link
Member Author

Created brave/brave-browser#5736 to track this problem; will fix ASAP 😄

bsclifton added a commit that referenced this pull request Sep 4, 2019
Default shield advanced view to TRUE for existing users.
bsclifton added a commit that referenced this pull request Mar 4, 2020
Logic introduced with #3154 did properly set the value for first run, but the value didn't seem to be persisted to profile. When quitting and relaunching, it would evaluate and make default advanced (not simple).

Since all users have a value for this (and most of them have it advanced = true), it's safe to roll this out. New users will have simple view by default.

Fixes brave/brave-browser#8533

-----

Revert "Merge pull request #3154 from brave/bsc-shields-advanced-default-change"

This reverts commit 590da54, reversing
changes made to b295203.
bsclifton added a commit that referenced this pull request Oct 13, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default shields mode depending on whether it is a new or existing Brave user
4 participants