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

Adds a session selector to bookmark dialog #14185

Closed
wants to merge 437 commits into from

Conversation

ishanrp
Copy link

@ishanrp ishanrp commented May 20, 2018

Fixes #7417

Adds a session selector to the add/edit bookmark dialog. When opened in a new tab the bookmark will be opened in the selected session.

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:

Adding using context menu

  1. Add new bookmark using the context menu.
  2. The current session should already be selected or select another session.
    Result: Opening the bookmark in new tab should open in the selected session.

add-bookmark

Using star to instant add

  1. Click on the bookmark star to quickly add the bookmark.
  2. The bookmark should be added with the current tab session.
    Result: Opening the bookmark in new tab should open it in the respective session.

add-using-star

Editing the bookmark

  1. Right click on any bookmark and click "Edit Bookmark".
  2. The edit bookmark dialog should open with the saved session selected.
  3. Change the selected session and save the bookmark.
    Result: Opening the bookmark in new tab should open in the selected session.

edit-bookmark

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

ryanml and others added 30 commits April 5, 2018 22:45
…n if there are no publishers in the ledger table.
Resolves brave#13754

Auditors:

Test Plan:
Fix brave#13689

This bug was caused because menu items were using a hidden window for their new tabs.

We should _not_ use `electron{.remote}.BrowserWindow.getAllWindows()` or `.get{Active, Focused}Window` directly. There are two reasons that could have bad results:
1. We create BrowserWindows which are not normal tabbed renderer windows (e.g. some code that runs on Bookmark creation)
2. We have a hidden window sometimes (the Buffer Window)
Instead call `app/browser/window.js`: `getAllRendererWindows` and `getActiveWindowId`.
Fix for main menu actions, which can be called when there are no windows, but when searching for `BrowserWindow.`

Test Plan:
- Check all menu items open new tab in currently active window
- Check all menu items open new tab in currently active window, when app is not focused
- Check all menu items open new tab in new window if there is no visible window
- Check tabs in all windows are persisted on restart when app / windows close
- Check process quits in Windows when last visible window is closed
Menu items which need a window will create / show one if needed
Set min-width of URL bar to 30px, so URL bar does not shrink
too much and URL bar elements are not layered on each other
when window width is reduced.

Fix brave#13773
Fix brave#13695

Remove usage of textCalc as it was the last usage of tabs.executeScriptInBackground which is not compatible with upcoming muon v6.x since as it incorrectly used executeScriptInTab which should only work on a WebContents which is a Tab, not a Window. Muon does not have a generic WebContents.executeJavascript method.

Also added bonus of not requiring creating new Windows in order to calculate text width and having to manage when they are closed, etc which should address the following issues, if that was the cause:
Fix brave#13422
Fix brave#13277
Calculate bookmarks bar overflow using css
fix brave#12671

test plan:
1. download an image and name it to rabbits.jpg
2. in the rabbits.jpg directory, start a localhost server: 'python -m SimpleHTTPServer 8000'
3. go to https://jsfiddle.net/c6y5qx5m/. you should see either 2 or
   3 copies of rabbits.jpg loaded.
4. go to about:preferences#security and enable 'Application Firewall'
5. go to https://jsfiddle.net/c6y5qx5m/ in a new private or session tab
   (to avoid loading cached files). now none of the rabbits.jpg images
   should load.
add off-by-default application firewall feature
fix brave#13668

Test Plan:
1. go to about:preferences#advanced
2. at the bottom, it should show a webrtc policy select menu which defaults
   to 'default'
3. turn on fingerprinting protection to 'block all'
4. go to https://browserleaks.com/webrtc. it should not show any IPs
5. turn off fingerprinting protection on that page. now it should show IPs
6. in about:preferences#advanced, set webrtc policy to 'default public
   interface only'
7. reload https://browserleaks.com/webrtc. it should only show the
   public IP.
8. in about:preferences#advanced, set webrtc policy to 'disable
   non-proxied UDP'
9. reload https://browserleaks.com/webrtc. it should show no IPs.
10. in about:preferences#advanced, set webrtc policy to 'default public
    and private interfaces'
11. reload https://browserleaks.com/webrtc. it should show both IPs.
Resolves brave#13531

Auditors:

Test Plan:
Reconcile is no longer triggered if there are no publishers in the ledger table.
Resolves brave#12792

Auditors:

Test Plan:
add advanced webrtc IP handling preference
fix brave#13805

Test Plan:
1. go to about:preferences > Advanced
2. you shouldn't see webrtc options there
3. go to about:preferences > security
4. you should see the webrtc options
Move webrtc options from advanced to security prefs
NejcZdovc and others added 18 commits May 13, 2018 08:12
…sort"

This reverts commit 12d502f, reversing
changes made to 0abdd64.
Revert "Merge pull request brave#13726 from NejcZdovc/fix/brave#13721-sort"
Fixes brave#12636

Auditors: @cezaraugusto

Test Plan:
1. Merge code
2. Re-push latest beta (`browser-laptop-release-linux` job in linux with `beta` param)
3. Re-push latest release (`browser-laptop-release-linux` job in linux with `release` param)
4. Verify packages are installable on Debian Buster
Resolves brave#13721

Auditors:

Test Plan:
fix: Ensure that only "What do these policies mean" is clickable and the entire row
…v-overlay

Fixed recovery overlay showing invalid amount before completion
Change facebook top tile to github and improve styling of top tiles icons
address brave#8956
-
this changes the options order from
allow/deny to deny/allow to match other notifcations
autoplay notification should follow spec
@ishanrp ishanrp changed the title Feature/bookmark session Fixes #7417 Adds a session selector to the add/edit bookmark dialog May 20, 2018
@ishanrp ishanrp changed the title Fixes #7417 Adds a session selector to the add/edit bookmark dialog Fixes #7417 Adds a session selector to bookmark dialog May 20, 2018
@ishanrp ishanrp changed the title Fixes #7417 Adds a session selector to bookmark dialog Adds a session selector to bookmark dialog May 20, 2018
@bsclifton bsclifton added this to the Completed work milestone Aug 13, 2018
@bsclifton bsclifton removed this from the Completed work milestone Sep 10, 2018
@NejcZdovc NejcZdovc removed their request for review September 18, 2018 08:30
@bsclifton
Copy link
Member

Thanks for the patch, @ishanrp! Sorry that we weren't able to review this in a timely manner ☹️ I'm going to close this PR out as I've migrated the issue to our new code-base (brave-core). If you haven't checked out that repo yet, please give it a look 😄 We also have a build available here:
https://brave.com/download-dev

@bsclifton bsclifton closed this Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.