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

Support opening new regular, private, private with Tor window in Tor window #3813

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Oct 29, 2019

Fix brave/brave-browser#6467
Fix brave/brave-browser#6646

Per slack discussion, our goal here is to let users open all types of new windows (regular / private / private with Tor) in the hamburger and file menu in regular, private, and Tor Windows.

As screenshot below, New Window, New Private Window, and New Private Window with Tor are shown in the hamburger menu of these three types of window, and it will open the corresponding type of new window through any of the options.

regular:
Screen Shot 2019-10-29 at 12 14 27 PM

private:
Screen Shot 2019-10-29 at 12 14 18 PM

private with Tor:
Screen Shot 2019-10-29 at 12 13 43 PM

Submitter Checklist:

Test Plan:

  1. Open a regular window, go through New Window, New Private Window, New Private Window with Tor in the hamburger menu, it should create the corresponding type of new window.
  2. Open a private window, go through New Window, New Private Window, New Private Window with Tor in the hamburger menu, it should create the corresponding type of new window.
  3. Open a Tor window, go through New Window, New Private Window, New Private Window with Tor in the hamburger menu, it should create the corresponding type of new window.
  4. If test under MacOS, repeat 1-3 but open them through the file menu this time instead of the hamburger menu.

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.

…window.

New window, New private window, New private window with Tor are shown in the
hamburger and file menu, and users could use them to open any type of new
window in Tor window.
@yrliou yrliou added this to the 0.73.x - Nightly milestone Oct 29, 2019
@yrliou yrliou self-assigned this Oct 29, 2019
@yrliou yrliou requested review from bsclifton and simonhong October 29, 2019 19:26
Comment on lines +31 to +34
if (brave::IsTorProfile(browser->profile())) {
chrome::NewEmptyWindow(browser->profile());
return;
}
Copy link
Member Author

@yrliou yrliou Oct 29, 2019

Choose a reason for hiding this comment

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

If we're already in Tor window, open a new empty Tor window.

Comment on lines +82 to +84
InsertItemWithStringIdAt(GetIndexOfCommandId(IDC_NEW_INCOGNITO_WINDOW) + 1,
IDC_NEW_OFFTHERECORD_WINDOW_TOR,
IDS_NEW_OFFTHERECORD_WINDOW_TOR);
Copy link
Member Author

@yrliou yrliou Oct 29, 2019

Choose a reason for hiding this comment

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

Show this option in Tor window too.

Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

@yrliou
Copy link
Member Author

yrliou commented Oct 30, 2019

CI failed on linux network audit with known error which is success in previous build.
Window is spending an extreme long time in dist and cannot resume at the moment probably because Jenkins was restarted during it.
I don't think this PR could cause dist fail on Windows only so I'm going to merge this without waiting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants