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

Fixes Tor avatar menu after c78 bump. #3686

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Fixes Tor avatar menu after c78 bump. #3686

merged 1 commit into from
Oct 14, 2019

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Oct 12, 2019

Also, removed no longer used override in chromium_src's profiles_state.cc.

Fixes brave/brave-browser#6451

Submitter Checklist:

Test Plan:

  1. Start Brave.
  2. Open Tor window.
  3. Verify that the avatar button in Tor window only has Tor icon.
  4. Click on the avatar button.
  5. Verify that the menu contains Tor and Exit Tor options.

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.

@mkarolin mkarolin added this to the 0.72.x - Nightly milestone Oct 12, 2019
@mkarolin mkarolin self-assigned this Oct 12, 2019
@mkarolin mkarolin changed the title Fixes Tor avatar menu after c78 bump. WIP: Fixes Tor avatar menu after c78 bump. Oct 12, 2019
@mkarolin mkarolin force-pushed the maxk-fix-tor-menu branch 2 times, most recently from 1512dc5 to 3106d7a Compare October 12, 2019 02:05
@bsclifton
Copy link
Member

Seeing an issue with this one on Private windows; it'll always show Tor in the profile menu with this change:
Screen Shot 2019-10-11 at 11 17 43 PM

Tor properly shows only Tor profile:
Screen Shot 2019-10-11 at 11 18 38 PM

@mkarolin
Copy link
Collaborator Author

@bsclifton, yep, I might have skipped a return 🤣

Also, removed no longer used override in chromium_src's profiles_state.cc.

Fixes brave/brave-browser#6451
@mkarolin mkarolin changed the title WIP: Fixes Tor avatar menu after c78 bump. Fixes Tor avatar menu after c78 bump. Oct 14, 2019
@mkarolin
Copy link
Collaborator Author

Only lint and unrelated browser test failed in CI.

@bsclifton
Copy link
Member

Changes work great 😄

While testing this, I noticed a weird regression - cc: @yrliou who has done Tor work in Nightly (it may be due to that work? or Chromium 78? not sure)

Basically, in Dev channel for example (C77), when in a private window with Tor, you're not given the option to create a regular private window

  • via the hamburger menu
    Screen Shot 2019-10-14 at 9 27 54 AM
  • via the menu (macOS)
    Screen Shot 2019-10-14 at 9 30 17 AM

On master/Nightly, when in a private window with Tor, you're shown the option to open a new private window. If you do choose this, it opens a private window with Tor, NOT a regular private window.

  • hamburger menu
    Screen Shot 2019-10-14 at 9 28 07 AM
  • via the menu (macOS)
    Screen Shot 2019-10-14 at 9 30 37 AM

@yrliou is this related to the work you did? Either way, I think I'll create an issue 😄

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changed LGTM! Created brave/brave-browser#6467 to track the unrelated issue I found

@bsclifton bsclifton merged commit 0870d1c into master Oct 14, 2019
@bsclifton bsclifton deleted the maxk-fix-tor-menu branch October 14, 2019 16:44
mkarolin pushed a commit that referenced this pull request Oct 14, 2019
Fixes Tor avatar menu after c78 bump.
mkarolin pushed a commit that referenced this pull request Oct 15, 2019
Fixes Tor avatar menu after c78 bump.
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.

Tor profile button now includes "private" (C78)
2 participants