Lazy load background tabs at app startup #553

ayoy · 2022-04-21T08:44:29Z

Task/Issue URL: https://app.asana.com/0/1177771139624306/1201065943414949/f
Tech Design URL:
CC: @mallexxx @tomasstrba

Description:
I added a mechanism that records selected tabs in chronological order and reloads them in that order at app startup.
The logic is as follows:

app launches
if there are no URL tabs or only 1 URL tab and it's currently selected, do nothing, don't even initialize
if current tab is a URL tab, wait until it finishes loading (or fails to load), otherwise proceed immediately
pick up to 3 most recently visited URL tab and reload them in background
as background tabs finish (or fail) loading, repeat previous step (keeping at most 3 concurrent loads) until 20 background tabs are loaded or there are no more URL tabs to load
as the user switches through tabs during lazy loading, record visited tabs and remove them from the lazy loading queue (they were visited manually which triggered a reload)
if 20 tabs were reloaded in background or there are no more not activated URL tabs, report finished work

Additional details:

TabLazyLoader is owned by TabCollectionViewModel which serves as its data source
When lazy loader reports completion, it gets deallocated
Lazy loader does not differentiate between successful and failed loads, it only records website load attempts and completion (regardless of the outcome) - it's therefore not affected by "no internet" scenario
the implementation is based on generic types to allow for more thorough unit testing
the mechanism is inspired by how Chromium-based browsers work, but magic numbers 3 and 20 mentioned above were picked by Gabriel (still subject to change) - you'll find more information in the Asana task.
OSLog.tabLazyLoading was added to help debugging lazy loading (disabled by default)
If there are more than 20 tabs at app startup, lazy loading starts with 10 tabs adjacent to the current one, before proceeding to 10 most recently selected.
Lazy loading is paused every time the user interacts with the current tab (triggers a navigation).

Steps to test this PR:
You may find this URL helpful when testing: http://tfc.home.pl/dev/dominik/index-delay.php?delay=4
It's a PHP script that loads a simple website after a delay specified in the parameter.
Also enable logging for better insights into what's happening.

Scenario 1:

Open several tabs, try with content-heavy websites such as Asana or news websites
Close the app and reopen
Observe tabs being reloaded in background (tab titles updating) only after the current tab finishes loading

Scenario 2:

Open 1 URL tab and Settings
Activate Settings
Close the app and reopen
Observe that the URL tab gets loaded in background right after the app starts

Scenario 3:

Open >20 URL tabs :)
Close the app and reopen
Observe that only 20 tabs are lazy loaded

Scenario 4:

Open some tabs
On app relaunch, while tabs are loaded, select some of them
Verify in the log that the tabs you selected were not lazy loaded (unless they started loading before being selected)

Scenario 5:

Open Settings, homepage, bookmarks, etc.
Observe that on app relaunch, no lazy loading is performed2.

Scenario 6:

Open some URL tabs, disconnect internet, reload the app
Observe that lazy loading is attempted, completed and the lazy loader is disposed of

...

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

Also wait for the current tab to finish loading before initiating lazy loading.

…elected

tomasstrba

Dominik, this looks good to me! 👍 Very nice code 🎖️ 🏅

Please, I will approve once we discuss suggestions I raised in Asana related to:

Pausing of the lazy loading if currently selected tab starts loading
Disabling of title changes while tab is being lazy loaded

tomasstrba · 2022-04-21T10:57:35Z

DuckDuckGo/Browser Tab/Model/Tab.swift

@@ -1091,6 +1097,7 @@ extension Tab: WKNavigationDelegate {
 // https://app.asana.com/0/1199230911884351/1200381133504356/f
 // hasError = true

+ webViewDidFailNavigationPublisher.send()


tomasstrba · 2022-04-21T11:06:16Z

DuckDuckGo/Tab Bar/ViewModel/TabCollectionViewModel.swift

@@ -59,6 +59,7 @@ final class TabCollectionViewModel: NSObject {

 // In a special occasion, we want to select the "parent" tab after closing the currently selected tab
 private var selectParentOnRemoval = false
+ private var tabLazyLoader: TabLazyLoader<TabCollectionViewModel>?


tomasstrba · 2022-04-21T11:10:32Z

DuckDuckGo/Tab Bar/ViewModel/TabLazyLoader/TabLazyLoader.swift

+import Combine
+import os
+
+final class TabLazyLoader<DataSource: TabLazyLoaderDataSource> {


Please, just to understand, what is the advantage of data source being generic parameter here?

The main reason for introducing generics to this code was increasing testability. It starts with Tab class which is hard to work with in unit tests, because it has an assert in deinit requiring to call tabWillClose() before deinitializing. It also contains a web view, etc.

But the lazy loader works on tabs via TabCollectionViewModel, and even if we created a data source protocol that TabCollectionViewModel would implement, that protocol would still make use of the concrete Tab type. And after all, the lazy loader only needs a few properties and functions from Tab, and we definitely don't want to deal with a real webView in unit tests.

Hence the LazyLoadable protocol and the use of associatedtype in the data source protocol. And that associated type makes it impossible to use TabLazyLoaderDataSource as a standalone type, producing the infamous error:

Protocol 'TabLazyLoaderDataSource' can only be used as a generic constraint because it has Self or associated type requirements

So for the lazy loader to define a data source of type TabLazyLoaderDataSource, it needs to become a generic class.

Bottom line - it was not my intention to make the code look sophisticated through the use of generics, but rather it was the only way to get the code to compile after abstracting the data source :)

Thanks! 🙏 More clear to me now :)

tomasstrba · 2022-04-21T11:23:30Z

DuckDuckGo/Tab Bar/ViewModel/TabLazyLoader/LazyLoadable.swift

+
+ var url: URL? { content.url }
+
+ var loadingFinishedPublisher: AnyPublisher<Tab, Never> {


… to current Load 10 adjacent tabs, based on index diff vs current tab: 1, -1, 2, -2, 3, etc. After that, load 10 recently selected tabs.

ayoy · 2022-04-21T15:49:43Z

I added loading adjacent tabs when there are more than 20 tabs, but this PR will keep evolving as I'm adding new features requested in the Asana task comments :) marking as draft for now.

ayoy · 2022-04-22T09:23:30Z

@tomasstrba the PR is ready for review and contains the following changes since last time you've checked it:

when there are more than 20 tabs, start with loading 10 tabs adjacent to the tab that was selected at app startup, before proceeding to loading 10 last selected tabs
tab title is not refreshed when it's lazy loaded - this limits the number of refreshes but does not eliminate them completely, since title can be updated by JS code after the tab has "fully" loaded according to webView
when current tab is reloaded, lazy loading is paused (new tabs are not selected for lazy loading until current tab stops reloading).

I will work on "Reopen Last Closed Window" in a separate pull request as I believe it is fully independent of these changes.

tomasstrba · 2022-04-22T09:36:06Z

Awesome! 👏 Will take a look at it soon. (Probably won't be part of the release today, but that is ok)

tomasstrba

LGTM! Nice work! 💯 I only have found last blocker that I would like to discuss. We currently trigger lazy loading for each window. Does it make sense to limit it to the main window?

Hypothetically, someone with 10 windows restored could have really slow start. WDYT?

tomasstrba · 2022-04-24T18:32:46Z

DuckDuckGo/Tab Bar/ViewModel/TabLazyLoader/TabLazyLoader.swift

+import Combine
+import os
+
+final class TabLazyLoader<DataSource: TabLazyLoaderDataSource> {


Thanks! 🙏 More clear to me now :)

ayoy · 2022-04-25T07:57:22Z

LGTM! Nice work! 💯 I only have found last blocker that I would like to discuss. We currently trigger lazy loading for each window. Does it make sense to limit it to the main window?

Hypothetically, someone with 10 windows restored could have really slow start. WDYT?

That's a very fair point Tom! I adjusted the code so that lazy loading is requested by WindowsManager only for the new key window while restoring state. Also added a guard that prevents scheduling lazy loading more than once for a TabCollectionViewModel instance.

Going forward, lazy loading can be expanded to cover multiple windows as a separate project, someday.

tomasstrba · 2022-04-26T14:52:35Z

Jumping on this for a final review

tomasstrba

LGTM! 👍

@mallexxx had a pretty good point in Asana, that if there is no connection after the start, tabs show "Oooops" title. I think it is fine to move on with this PR but do you mind scoping a follow-up task solving this issue?

ayoy · 2022-04-26T15:09:04Z

Thank you very much Tom! Yes, I will add a follow-up task according to the comment from @mallexxx.

# By Alexey Martemyanov (3) and others # Via GitHub * develop: Lazy load background tabs at app startup (#553) Update the Fireproof checkmark in the Save Credentials view controller (#555) Support config v2 (#528) Fullscreen video fixing (#541) Add data import failure pixels (#552) Update BSK to fix autofill on Catalina (#551) fix contrast bug on Catalina / Big Sur (#546) Disable download reload on page tab reactivation/session restoration (#516) Add "New Window" item to App Dock menu (#544) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj

ayoy added 17 commits April 20, 2022 14:29

Add Tab.lastSelectedAt and reload 3 last selected tabs at startup

673f12a

Add TabLazyLoader class that handles reloading

4b6d58d

Also wait for the current tab to finish loading before initiating lazy loading.

Reload 3 background tabs one by one

7b7fc2b

Lazy load tabs all at once the current one has loaded

d2ef4c2

Clean up implementation and dispose of Lazy Loader once finished

0e440f0

Add lazy loading logger

123e70c

Fix lazy loading when there's only 1 URL tab and it's not currently s…

f71f49c

…elected

Improve disposing of Tab Lazy Loader

544bc49

Prepare TabLazyLoader for testing

8a37b5b

Add initial unit tests for TabLazyLoader

7289b2b

Force a full reload of background tabs

69959f7

Fix tabDidFinishLazyLoadingSubject

65d538e

Lazy load up to 20 tabs, up to 3 at once

671c373

Clean up implementation

51d5848

Track selected Tab IDs instead of Tabs in Tab Lazy Loader

58731b0

Make TabLazyLoader generic and add unit tests

1e3af6c

Move TabLazyLoader.Const to the top of the class

5c64229

tomasstrba self-requested a review April 21, 2022 10:56

tomasstrba self-assigned this Apr 21, 2022

tomasstrba reviewed Apr 21, 2022

View reviewed changes

tomasstrba assigned ayoy and unassigned tomasstrba Apr 21, 2022

ayoy added 3 commits April 21, 2022 14:14

Merge branch 'develop' into dominik/lazy-load-tabs

e59a4da

Add AdjacentItemEnumerator

9271fb1

If there are more than 20 tabs, start lazy loading with tabs adjacent…

a67d125

… to current Load 10 adjacent tabs, based on index diff vs current tab: 1, -1, 2, -2, 3, etc. After that, load 10 recently selected tabs.

ayoy added the draft label Apr 21, 2022

ayoy added 3 commits April 22, 2022 00:08

Add a unit test for loading adjacent tabs

bcf1977

Skip Tab title updates during lazy loading

4691432

Pause lazy loading whenever currently selected tab is loading

fb26ac9

ayoy added 2 commits April 22, 2022 10:45

Disable lazy loader logging

d450953

Add documentation for AdjacentItemEnumerator

d4e5ff8

ayoy removed the draft label Apr 22, 2022

Count number of adjacent tabs to load from max to 0

271ee95

ayoy assigned tomasstrba and unassigned ayoy Apr 22, 2022

Skip Tab favicon updates during lazy loading

3ac486a

tomasstrba requested changes Apr 24, 2022

View reviewed changes

tomasstrba assigned ayoy and unassigned tomasstrba Apr 24, 2022

Apply lazy loading only for the key window

3699edf

ayoy force-pushed the dominik/lazy-load-tabs branch from a56acba to 3699edf Compare April 25, 2022 07:53

ayoy assigned tomasstrba and unassigned ayoy Apr 25, 2022

ayoy requested a review from tomasstrba April 25, 2022 07:57

tomasstrba approved these changes Apr 26, 2022

View reviewed changes

tomasstrba assigned ayoy and unassigned tomasstrba Apr 26, 2022

ayoy merged commit a750777 into develop Apr 26, 2022

ayoy deleted the dominik/lazy-load-tabs branch April 26, 2022 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load background tabs at app startup #553

Lazy load background tabs at app startup #553

ayoy commented Apr 21, 2022 •

edited

Loading

tomasstrba left a comment

tomasstrba Apr 21, 2022

tomasstrba Apr 21, 2022

tomasstrba Apr 21, 2022

ayoy Apr 21, 2022 •

edited

Loading

tomasstrba Apr 24, 2022

tomasstrba Apr 21, 2022

ayoy commented Apr 21, 2022 •

edited

Loading

ayoy commented Apr 22, 2022 •

edited

Loading

tomasstrba commented Apr 22, 2022

tomasstrba left a comment

tomasstrba Apr 24, 2022

ayoy commented Apr 25, 2022

tomasstrba commented Apr 26, 2022

tomasstrba left a comment

ayoy commented Apr 26, 2022


		var url: URL? { content.url }

		var loadingFinishedPublisher: AnyPublisher<Tab, Never> {

Lazy load background tabs at app startup #553

Lazy load background tabs at app startup #553

Conversation

ayoy commented Apr 21, 2022 • edited Loading

Internal references:

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba Apr 21, 2022

Choose a reason for hiding this comment

tomasstrba Apr 21, 2022

Choose a reason for hiding this comment

tomasstrba Apr 21, 2022

Choose a reason for hiding this comment

ayoy Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

tomasstrba Apr 24, 2022

Choose a reason for hiding this comment

tomasstrba Apr 21, 2022

Choose a reason for hiding this comment

ayoy commented Apr 21, 2022 • edited Loading

ayoy commented Apr 22, 2022 • edited Loading

tomasstrba commented Apr 22, 2022

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba Apr 24, 2022

Choose a reason for hiding this comment

ayoy commented Apr 25, 2022

tomasstrba commented Apr 26, 2022

tomasstrba left a comment

Choose a reason for hiding this comment

ayoy commented Apr 26, 2022

ayoy commented Apr 21, 2022 •

edited

Loading

ayoy Apr 21, 2022 •

edited

Loading

ayoy commented Apr 21, 2022 •

edited

Loading

ayoy commented Apr 22, 2022 •

edited

Loading