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

Toolbar: hide the profile button if there is only 1 user profile #3097

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

petemill
Copy link
Member

@petemill petemill commented Aug 6, 2019

Fix brave/brave-browser#5091

Submitter Checklist:

Test Plan:

New automated browser tests also cover most of the following...

  1. Fresh profle

  2. Open 1 or more windows
    --> Profile button is now shown

  3. Open the guest window
    --> Profile button is shown

  4. Open incognito / tor window
    --> Profile button is shown

  5. Create a new profile (on macos use the System menu, on other OS wait until App Menu gets Create Profile and Switch to Guest Profile items #3096 provides the app menu action!)
    ---> Profile button is shown on original profile window
    ---> Profile button is shown on new profile window

  6. Delete profile (click on profile button, then Manage profiles, then the menu button on one of the profiles, then remove profile, then confirm!)
    ---> Profile button is not shown on existing remaining profile window

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.

@petemill petemill self-assigned this Aug 6, 2019
@@ -136,6 +152,16 @@ void BraveToolbarView::OnThemeChanged() {
bookmark_->UpdateImage();
}

void BraveToolbarView::OnProfileAdded(const base::FilePath& profile_path) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this callback only called only when new normal profile is created?

Copy link
Member Author

Choose a reason for hiding this comment

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

@simonhong I don't see this getting called when starting / opening a guest or tor profile. Are there any other non-normal profiles I should check?

Copy link
Member

Choose a reason for hiding this comment

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

If we don't need to check about tor/guest profile add/delete, I think current impl is fine.
FYI, we can get all profile creation by NOTIFICATION_PROFILE_XXX notifications.

Profile* profile = browser()->profile();

// Track changes in profile count
profile_observer_.Add(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we skip this for Tor/Guest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

Copy link
Member Author

Choose a reason for hiding this comment

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

done @mkarolin

@petemill petemill force-pushed the profile-toolbar-hide branch from 23e1968 to 45a03ee Compare August 7, 2019 19:01
@petemill petemill requested review from simonhong and mkarolin August 7, 2019 20:32
@petemill
Copy link
Member Author

petemill commented Aug 7, 2019

Feedback addressed

@petemill petemill force-pushed the profile-toolbar-hide branch from 45a03ee to 2375d4c Compare August 7, 2019 21:03
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

++

@petemill
Copy link
Member Author

petemill commented Aug 8, 2019

CI did pass

@petemill petemill merged commit efd630b into master Aug 8, 2019
@petemill petemill deleted the profile-toolbar-hide branch August 8, 2019 16:32
@petemill petemill added this to the 0.70.x - Nightly milestone Aug 9, 2019
@bsclifton
Copy link
Member

When testing #3140, I realized that this might be hiding one of the main ways to ADD a user. macOS does have a People menu, but I didn't test on Windows (which doesn't have menus).

I think we may need to add the Manage people link to our brave://settings page as a replacement
Screen Shot 2019-08-12 at 9 23 50 AM

cc: @rebron

@petemill
Copy link
Member Author

@bsclifton this was added to the app menu for all platforms in brave/brave-browser#5212

@bsclifton bsclifton mentioned this pull request Aug 12, 2019
32 tasks
@rebron
Copy link
Collaborator

rebron commented Aug 12, 2019

Create a New Profile in the app menu is the way to add a new profile. With just one profile, we could have 'Manage Profiles' in Settings though it's not necessarily intuitive to Add a user that way. It would be go to Settings, Manage Profile, Add a Profile versus going to App menu and selecting Create a New Profile.

Not sure we need a second entry point.

@bsclifton
Copy link
Member

@rebron @petemill good call- totally missed that! 😄 Yup, we're G2G
Screen Shot 2019-08-12 at 9 34 12 AM

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.

Hide profile icon in navigation bar until user creates a second profile
5 participants