Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #7574: Add support for multi-window & persistent private browsing #7575

Merged
merged 13 commits into from
Aug 4, 2023

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Jun 6, 2023

Security Review

Summary of Changes

  • Add support for multi-window
  • Add "Open In New Window" and "Open In New Private Window" context menu when long-pressing on a link.
  • Add support for persistent private browsing (coming in a separate "commit").
  • Added support for Persistent Private Browsing (via a toggle in Privacy Settings), so private tabs can be session restored like Safari and all other browsers.
  • Private Browsing ONLY mode will automatically disable persistent private browsing!
  • Tabs do NOT save in PBO mode! Turning off PBO will still wipe all your tabs.
  • Turning off Persistent Private Browsing (PPB) will not wipe your tabs, but it will wipe them from DISK. So killing the app will not restore them.

TODO:

  • Need a better way to "open a new window" (instead of long-pressing the plus sign, but that's about it, and does not block review).

This pull request fixes #7574

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New or updated UI has been tested across:
    • Light & dark mode
    • Different size classes (iPhone, landscape, iPad)
    • Different dynamic type sizes

Test Plan:

  • Test every feature
  • Test private browsing for everything

Screenshots:

Split (Regular + Regular):
image

Split (Regular + Private):
image

Private Window Protection:
image

Private Window NTP:
image

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@Brandon-T Brandon-T added this to the 1.53 milestone Jun 6, 2023
@Brandon-T Brandon-T self-assigned this Jun 6, 2023
@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 2 times, most recently from f8269dd to 740f911 Compare June 9, 2023 19:06
@Brandon-T Brandon-T changed the title Fix #7574: Add support for multi-window Fix #7574: Add support for multi-window & persistent private browsing Jun 9, 2023
@Brandon-T Brandon-T marked this pull request as ready for review June 9, 2023 19:11
@Brandon-T Brandon-T requested a review from a team as a code owner June 9, 2023 19:11
@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 2 times, most recently from 55c3cff to c0c2ad2 Compare June 13, 2023 18:51
App/iOS/Delegates/AppDelegate.swift Outdated Show resolved Hide resolved
App/iOS/Delegates/SceneDelegate.swift Outdated Show resolved Hide resolved
App/iOS/Delegates/SceneDelegate.swift Show resolved Hide resolved
Sources/Brave/Frontend/Browser/TabManager.swift Outdated Show resolved Hide resolved
Sources/Brave/States/AppState.swift Outdated Show resolved Hide resolved
Sources/Brave/States/AppState.swift Outdated Show resolved Hide resolved
Sources/Brave/States/AppState.swift Outdated Show resolved Hide resolved
Sources/Brave/States/BrowserState.swift Outdated Show resolved Hide resolved
Sources/Brave/States/BrowserState.swift Outdated Show resolved Hide resolved
@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 2 times, most recently from 1c12673 to 71ae5ea Compare June 19, 2023 14:41
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Runtime Review:

Prefix: These were all tests based on having 2 windows of Brave side-by-side on a 12.9" iPad in landscape orientation

Bugs That Need Fixing:

  • Brave News: Turning on Brave News on one windows NTP breaks Brave News on the other window

    • Expected: News should load on the other window as well and display the same data set
  • Brave News: Altering followed sources only brings up "New Content Available" button on window that triggered it. Closing a tab on another window doesn't reload Brave News with the new set of data and will not show the updated data until the app is killed and re-launched.

    • Expected: Both NTPs should reference the same set of News data or at least ensure that new NTP's use the updated data
  • Browser Lock: "Unlock" button only appears on 1 window until its tapped on the other?

    • Expected: Should show it on both probably

Simulator Screenshot - iPad Pro (12 9-inch) (6th generation) - 2023-06-19 at 15 51 07

  • Playlist: Plays the same content on multiple windows

    • Expected: If content can only be played on one window at a time then we need to block the other or something with some UI
  • Playlist: Shows "Resource Loading" errors when playing from multiple windows

Polish:

  • Shields: Turning shields off for a website loaded in multiple windows does not reflect that

    • Expected: Sync state across windows, reload matching webpages on all connected scenes?
  • Brave News: Toggling follow status of a source while viewing the source window on both windows don't update until you leave and revisit the screen

    • Expected: If possible these should be synced so that changes to the
  • Browser Lock: Unlocking from browser lock only unlocks one window. Intended behaviour?

    • Expected: Unlock all scenes that are connected/visible on the screen/foregrounded
  • Playlist: Removing an item from Playlist doesn't update the URL bar if you're viewing the videos source webpage at the same time

    • Expected: Should stay in sync so that items added/removed from one window sync to the other
  • Tab Tray: For some reason if you open the tab tray on one window and click on the other window, the cells aspect ratio changes.

    • Expected: I assume the longer one is the correct one but not sure

Simulator Screenshot - iPad Pro (12 9-inch) (6th generation) - 2023-06-19 at 16 09 13

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

This PR may as well been named "Remove PrivateBrowsingManager singleton, oh and by the way also add multi-window support" 😂

@Brandon-T
Copy link
Collaborator Author

Brandon-T commented Jun 20, 2023

Bugs That Need Fixing

  • Fixed Brave-News not syncing across windows by using the same data-source for all (fixes all the Brave-News issues mentioned).
  • Fixed playlist url bar not syncing across windows when watching the same video in two windows, and adding/removing from playlist in one of the windows.
  • Fixed turning off/on shields on one window with the same tab open on another window, not syncing across windows. This one was weird to fix, because technically turning off the shields on one tab ("youtube" for example), would NOT reload all the "youtube" tabs that are open. It only ever reloaded the "selected" tab. But I fixed it so it reloads ALL of the tabs that are of the same domain, across ALL of the windows. I wasn't sure of the behaviour :D so I went with reloading all.

Not sure:
I'm not sure how the Browser-Lock should behave tbh.
For example, what if I locked one window because it's private, but then leave another window unlocked? Or what if I minimized one window which causes it to lock, it shouldn't lock the other window that's in the foreground, so I would expect to unlock each window?

Also, I left playlist working on multiple windows because I realized this is what Safari and WebKit do when you're playing Youtube on multiple web-views. So I followed the same behaviour. Though, I could always block it I guess.

@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 2 times, most recently from de20f9b to ea30164 Compare June 27, 2023 18:08
@aguscruiz
Copy link
Collaborator

@iccub
Copy link
Contributor

iccub commented Jul 3, 2023

Thanks @aguscruiz.
This is however separate from what's left for this PR which is a small alert telling existing users whether they want to persist their tabs or not

@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 3 times, most recently from c4fa06e to 922a481 Compare July 6, 2023 16:47
@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 4 times, most recently from 0af679b to e528998 Compare July 17, 2023 21:37
@iccub iccub modified the milestones: 1.56, 1.57 Jul 28, 2023
@Brandon-T Brandon-T force-pushed the feature/MultiWindow branch 2 times, most recently from 78573f1 to 8c3d685 Compare August 3, 2023 17:23
Brandon-T and others added 13 commits August 4, 2023 13:12
Fix BraveRewards crash when allocating multiple rewards.
Add persistent private browsing.
Wallet fix for when onboarding is visible in multiple windows.

Fix windowIds being the same on tapping the app icon.
Set window selected
Fix window activation predicate so that tapping the Application Icon does not create new windows.

Added documentation to the SceneDelegate when restoring.
Fix private tab deleting when exiting persistence mode.
Move AppState to the same location as delegates
Open context menus like Safari to allow opening URLs in new windows
Moved alert for existing users and updated text.
Updated icons with leo-sfsymbol
Rebase & Fixed bug in alert
Default persistent private browsing to off.
@iccub iccub merged commit 63a16a3 into development Aug 4, 2023
7 checks passed
@iccub iccub deleted the feature/MultiWindow branch August 4, 2023 17:48
arthuredelstein pushed a commit to brave/brave-core that referenced this pull request Feb 13, 2024
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.

Add support for multi-window
6 participants