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

Re-order brave menus at app menu #4235

Merged
merged 2 commits into from
Dec 18, 2019
Merged

Re-order brave menus at app menu #4235

merged 2 commits into from
Dec 18, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Dec 15, 2019

Insert & reorder brave menus based on corresponding commands enable status.
If we want to add/remove from app menu, adjust enable commands status
instead of adjusting it in app menu model directly.
App menu model will add/delete based on commands status.

Fix brave/brave-browser#5552
Normal window app menu
Screen Shot 2019-12-15 at 11 43 49 AM
Private window app menu
Screen Shot 2019-12-15 at 11 44 18 AM
Tor window app menu
Screen Shot 2019-12-15 at 11 44 34 AM
Guest window app menu
Screen Shot 2019-12-15 at 11 44 50 AM

Submitter Checklist:

Test Plan:

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.

@simonhong simonhong added this to the 1.4.x - Nightly milestone Dec 15, 2019
@simonhong simonhong self-assigned this Dec 15, 2019
@simonhong
Copy link
Member Author

@rebron I added Ad block as last because corresponding issue doesn't specify it.
Please comments if it needs re-position.

@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows and removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows labels Dec 15, 2019
@rebron
Copy link
Collaborator

rebron commented Dec 16, 2019

@simonhong Brave Ad Block is good as last on that list. I'll update the issue.

@bsclifton
Copy link
Member

Linux got the 03:36:01 /home/ubuntu/jenkins/workspace/rowser-build-pr_reorder_app_menu/src/out/Release/brave_browser_tests: /lib/x86_64-linux-gnu/libm.so.6: version GLIBC_2.29' not found (required by /home/ubuntu/jenkins/workspace/rowser-build-pr_reorder_app_menu/src/out/Release/brave_browser_tests)` error

and Windows had a build error; going to restart CI for those two platforms

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Dec 16, 2019
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.

Tested + changes work great; code changes LGTM (pending CI) 👍

@simonhong
Copy link
Member Author

Hmm, still glibc issue on linux.

Insert & reorder brave menus based on corresponding commands enable status.
If we want to add/remove from app menu, adjust enable commands status
instead of adjusting it in app menu model directly.
App menu model will add/delete based on commands status.

Fix brave/brave-browser#5552
and cleanup brave app menu test.
@simonhong simonhong merged commit 2d4e5bf into master Dec 18, 2019
@simonhong simonhong deleted the reorder_app_menu branch December 18, 2019 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reorder items in the App menu
4 participants