Pinned tabs

[ayoy]

Task/Issue URL: https://app.asana.com/0/0/1187695805145593/f
Tech Design URL:

• https://app.asana.com/0/0/1202523488416988/f
• https://app.asana.com/0/0/1202524827981422/f

Description:
Pinned tabs are shared between windows. As such, the TabCollection, TabViewModels
and Tabs live in a PinnedTabsManager instance owned by WindowControllersManager.
Even though pinned tabs collection is the same for every window, the selection
is local to window and interferes with window's unpinned tab collection.
Hence, every TabCollectionViewModel contains a reference to pinned tabs manager and:

• still keeps only unpinned tabs (and their view models) in its own TabCollection,
• keeps delegate callbacks only for events related to unpinned tabs (the delegate
  is used to update the unpinned tabs collection view),
• can point to a pinned tab via selectionIndex and selectedTabViewModel,
• some of its public API can work on pinned tabs collection (selecting, inserting,
  removing, duplicating and replacing).

TabIndex enum was introduced to describe tabs in the tab bar area (both unpinned
and pinned). Pinning and unpinning happens via TabCollectionViewModel API as well.

Since pinned tabs are shared between windows, so are their webviews, and it becomes
a bit tricky with multiple windows and selecting the same pinned tab in more than
one window. BrowserViewController intercepts window become/resign key events and
performs additional logic to replace a webview with its snapshot as needed. It gets
even more complicated because WKWebView contents can only be snapshotted using
a dedicated, asynchronous method. Some glitches can be observed, but Safari is not
much better at it. This has become even more tricky to handle at application startup,
so I ended up not persisting pinned tab selected index and default to first unpinned
tab in such situation (that's also in line with Safari).

Navigating from a pinned tab to a different domain always opens a new unpinned tab,
hence some additional logic inside Tab.webView(_:decidePolicyFor:).

Tab Lazy Loading starts with pinned tabs when there are any, before moving on to
other tabs.

Fire button action does not work on pinned tabs, but they are not explicitly listed
anywhere (only the information about pinned tabs is displayed). This is achieved
by fireproofing pinned tabs' domains right before the fire action and removing
fireproofing right after. This can of course cause pinned tabs to become permanently
fireproof if you quit the app while burning, but we have an even more serious privacy
issue related to that.

Steps to test this PR:

1. Open the app with a single URL tab, pin the tab and verify that it's pinned and an unpinned home page is added.
2. Try cmd+w on a pinned tab and verify that it's not closed and instead the first unpinned tab is selected
3. Pin a couple of tabs and drag them around to reorder; use cmd+{} to navigate through pinned (and unpinned) tabs and verify that it works properly
4. Pin a website such as https://news.ycombinator.com; verify that links to other websites open in new unpinned tabs and links to the same domain (Ask, Show, etc.) open in the same pinned tab.
5. Open more than one window (preferably more than two) and click through windows and tabs; verify that it doesn't break.
6. Disable session restoring and close the app with pinned tabs; verify that pinned tabs are restored even though the session is not.
7. Verify that lazy loading works for pinned tabs. Launch the app, wait for a couple of seconds and visit pinned tabs to see that they have been preloaded.
8. Test context menu actions for pinned tab (NB that duplicate action creates a new pinned tab).

Testing checklist:

• Test with Release configuration
• Test proper deallocation of tabs
• Make sure committed submodule changes are desired

---

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

[samsymons]

This looks good. The only thing I've seen so far is that the snapshotting system doesn't cope as well with two windows of vastly different aspect ratios. Otherwise, the code is great, and the feature itself is awesome. 👌

[ayoy]

Hi @samsymons! Thank you very much for the review. The problem you highlighted is actually a regression and it's caused by adding .receive(on: DispatchQueue.main) in the subscription related to snapshotting a webview. I'm glad you caught it, because I had spent several hours trying to limit the occurrence of this glitch.

I missed the fact that adding .receive(on: DispatchQueue.main) makes the sink function dispatched asynchronously at all times (in my defense, this has not been the case with RxSwift that I'm used to :)). It seems that we can't afford to receive the subscription asynchronously because the webview is already in the other window and has a different frame.

I will remove the .receive call and add dispatchPrecondition to the actual snapshotting method, so that we know when somebody calls it from a background thread in the future. I hope that's okay.

With this change, the glitch you reported will only be visible in the following scenario:

1. open 2 windows with pinned tabs, both showing another (e.g. unpinned tab)
2. select pinned tab in one window
3. click the same pinned tab in the other window, causing the window to become key and immediately select the pinned tab.
4. it glitches here once, and if you continue to switch between windows in this state, it won't be glitching anymore.

Other cases would work fine, feel free to test it after my change :)

[samsymons]

Re-approved 🎉

[ayoy]

Hi @samsymons! There are some small changes that I had to make as a result of the Product Review. It's mainly removing previous changes:

• "Pinned tabs won't be cleared" label
• no more implicit pinned tabs fireproofing
• adjustments to support burning pinned tabs.

It would be great if you could have another look. Many thanks 🙌

[samsymons]

Neat, I was able to see the constraint visualizer in action. 🙂 Not through any error on your part, this is an existing problem. I actually have all sorts of problems with this pop-up window, including a possible retain cycle... I'll write it up. That's all unrelated to the new changes and just some things I noticed while testing, the new code looks good. 👍

[samsymons]

New changes and behaviour look good, nice job getting that done with a short turnaround time. 👍