Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

add tor menu items, make tor switch per tab #14467

Merged
merged 4 commits into from
Jun 21, 2018
Merged

add tor menu items, make tor switch per tab #14467

merged 4 commits into from
Jun 21, 2018

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Jun 20, 2018

fix #14459

test plan:

  1. every menu that says 'open in new private tab' should also have an
    item that says 'open in new tor tab'
  2. opening in a new tor tab should show the tor indicator on
  3. opening in a new private tab should show the tor indicator off
  4. ddg is enabled by default in tor tabs but can be turned off in about:preferences#search
  5. cmd+click should open in the same type as the current tab
  6. turning the tor switch on/off in a new tab should work
  7. in a tor tab, right-click should show 'open in new tor tab' first

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

fix #14459

test plan:
1. every menu that says 'open in new private tab' should also have an
   item that says 'open in new tor tab'
2. opening in a new tor tab should show the tor indicator on
3. opening in a new private tab should show the tor indicator off
4. ddg is enabled by default for now but can be turned off
5. cmd+click should open in the same type as the current tab
6. turning the tor switch on/off in a new tab should work
7. in a tor tab, right-click should show 'open in new tor tab' first
@@ -151,7 +151,7 @@ module.exports = {
'general.spellcheck-languages': Immutable.fromJS(['en-US']),
'search.default-search-engine': 'Google',
'search.offer-search-suggestions': false, // false by default for privacy reasons
'search.use-alternate-private-search-engine': false,
'search.use-alternate-private-search-engine': true,
Copy link
Member Author

Choose a reason for hiding this comment

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

debatable; alternative (can be done in follow-up) is to make DDG a per-tab setting like Tor

Test Plan: Check that the DDG switch in regular private tabs doesn't
affect tor tabs and that the DDG switch in about:preferences for Tor
tabs doesn't affect regular private tabs. Also check that DDG is default
on for tor tabs.
* change "make private tabs" to "make this tab"
* move DDG section below tor section in about:newtab
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.

This is great. I especially like that the 'Open this link in New Private Tab [with Tor]' menu items are re-ordered if the current tab is / is not Tor.

Spotted one functional issue, and one wording question.

ensureAtLeastOneWindow({
url: 'about:newtab',
isPrivate: true,
isTor: true
Copy link
Member

Choose a reason for hiding this comment

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

This will not result in a tor tab when there are 0 windows open, which is a common macOS scenario (as judging from the number of issues that were raised when I had a similar bug!).

In order to fix this we simply need to make the openFramesInWindow function inside app/browser/windows.js aware of the tor: property for the createTabRequested options argument.

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 catch thanks!

@@ -27,6 +27,8 @@ privateTabText1=Whether or not you use Tor, private tabs are not logged in page
privateTabTitle=This is a Private Tab
privateTabTorTitle=Make private tabs much more private with
Copy link
Member

Choose a reason for hiding this comment

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

With this change, perhaps we should not make it seem like a global setting a more like a setting just for the current tab: 'Make this tab much more private with Tor'?

template.push(
openInNewPrivateTabMenuItem(link, frame.get('tabId')),
openInNewPrivateTabMenuItem(link, frame.get('tabId'), isTor),
openInNewPrivateTabMenuItem(link, frame.get('tabId'), !isTor),
openInNewWindowMenuItem(link, frame.get('isPrivate'), frame.get('partitionNumber')),
Copy link
Member Author

Choose a reason for hiding this comment

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

bug: this should send isTor as the last argument

1. opening a new tor tab with no open windows should open a window with
   a tor tab
2. right-click 'open in new window' from a tor tab should open the link
   in a new window with a tor tab
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.

It all works great

@diracdeltas diracdeltas merged commit 6e228d9 into tor/0.23.x Jun 21, 2018
@diracdeltas diracdeltas deleted the fix/14459 branch June 21, 2018 02:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants