Allow automated fetching of synced bookmarks' favicons #564

ayoy · 2023-11-15T14:25:47Z

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/0/1205949780297088/f
iOS PR: duckduckgo/iOS#2163
macOS PR: duckduckgo/macos-browser#1857
What kind of version bump will this require?: Major

Tech Design URL: https://app.asana.com/0/481882893211075/1204986998781220/f

Description:
Add BookmarksFaviconFetcher that is used to fetch favicons for bookmarks received by Sync.
Fetcher is opt-in, controlled by a setting inside Sync settings (with an additional in-context onboarding
popup presented from client apps). Fetcher uses LinkPresentation framework to obtain a favicon
for a given domain, and in case of failure it falls back to checking hardcoded favicon URLs.
Fetcher keeps a state internally, by saving list of bookmarks IDs that need processing to a file on disk.
Fetcher plugs into clients' implementation of favicon storage by exposing FaviconStoring protocol.
Fetcher performs fetching on a serial operation queue. Each fetcher invocation cancels previously scheduled
operation and schedules a new one. Updating fetcher state is also scheduled on the operation queue -
state updates don't support cancelling and always finish before next operation is started.

Steps to test this PR:

Run the macOS app from Xcode, enable Sync logging in Debug -> Logging -> Sync.
Import some bookmarks (you can use a HTML file provided in the test builds task - it adds a bunch of popular websites as favorites).
Start sync, run the iOS app and connect to the Sync account – enable sharing favorites so that iOS gets the same set as macOS.

On iOS:

On iOS, go back from Sync settings to the home screen. Drag the favorites view or open a new tab. Verify that you're prompted to enable favicons fetching.
Enable favicons fetching and observe that favicons are being fetched (you need to drag the view to trigger reloading, or open new tab page to reload the full view. Some of the favicons in the test set won't be fetched because bookmarks URLs are not found (e.g. https://www.dropboxusercontent.com)
Kill the app while fetching and restart. Verify that fetching is resumed.
Disable Sync. Verify that fetching is interrupted.
Re-enable Sync. Verify that fetching is disabled.
Go to Debug menu -> Sync -> Reset favicons fetcher onboarding.
Go to bookmarks view and scroll to a bookmark without favicon. Verify that you're prompted to enable fetching favicons.
Verify that after all favicons are fetched, fetcher doesn't try to fetch any more favicons after every sync operation (the fetching operation should start and immediately finish).

On macOS:

After enabling Sync on iOS, go back to new tab page. Verify that you're prompted to enable favicons fetching. Note that the popup UI is not final.
Enable favicons fetching and observe that favicons are being fetched (you need to drag the view or switch tabs back and forth to trigger reloading).
Kill the app while fetching and restart. Verify that fetching is resumed.
Disable Sync. Verify that fetching is interrupted.
Re-enable Sync. Verify that fetching is disabled.
Go to Debug menu -> Sync -> Reset favicons fetcher onboarding.
Open Bookmarks Management view. Verify that you're prompted to enable fetching favicons.
Verify that after all favicons are fetched, fetcher doesn't try to fetch any more favicons after every sync operation (the fetching operation should start and immediately finish).

OS Testing:

Internal references:

Software Engineering Expectations
Technical Design Template

…c favorites

…Model

…wModel

…el and MenuBookmarksViewModel

…sInteracting

…cable settings

…tity unaware of display mode

…folder IDs from Constants

bwaresiak · 2023-11-22T15:54:23Z

Sources/Bookmarks/FaviconsFetcher/BookmarksFaviconsFetcher.swift

+
+        context.performAndWait {
+            let bookmarks = BookmarkUtils.fetchAllBookmarks(in: context)
+            ids = bookmarks.compactMap(\.uuid)


Maybe it would be more efficient to use dictionary result type query to get one property?

Great idea :) let me rework it

It's done! All other comments are addressed now so ready for you to take another look.

bwaresiak · 2023-11-22T16:05:25Z

Sources/Bookmarks/FaviconsFetcher/FaviconsFetchOperation.swift

+
+    // MARK: - Overrides
+
+    override var isAsynchronous: Bool { true }


Do we need this? You seem to be always starting operation through the queue?

I think we need this in order for the operation's main() function to not report finished state after exiting, but wait until isFinished is set by an async Task that's run there. It's the same for SyncOperation.

I don't think it hurts, but I belive this value is ignored by OperationQueue - as it always starts the operation on a queue. Nonetheless we can keep it. :)

Sources/Bookmarks/FaviconsFetcher/FaviconsFetchOperation.swift

bwaresiak · 2023-11-22T16:12:22Z

Sources/SyncDataProviders/Bookmarks/internal/BookmarksResponseHandler.swift

-//            guard let favoritesFolder = BookmarkUtils.fetchFavoritesFolder(context) else {
-//                // Error - unable to process favorites
-//                return
-//            }


What's with this? :)

It's a remainder from pre-form-factor-specific favorites code which I forgot to remove :)

…Utils.fetchAllBookmarksUUIDs

SabrinaTardio

Looks great and works perfectly! Great job
I left a couple of minor comments

Also general question… are we using different icons in the iOS prompt and macOS prompt

SabrinaTardio · 2023-11-28T13:08:33Z

Sources/Bookmarks/FaviconsFetcher/BookmarksFaviconsFetcher.swift

+}
+
+/**
+ * This class manages fetching favicons for bookmarks updated by Sync.


should we keep this generic and not relate it to sync specifically

The actual API technically doesn't prevent this fetcher to be used outside of Sync. I think it's okay to be specific about it in the docs at the moment, and update the docs whenever we apply the fetcher to another domain (which may require revisiting its API anyway).

SabrinaTardio · 2023-11-28T16:53:19Z

Sources/Bookmarks/FaviconsFetcher/BookmarksFaviconsFetcherStateStore.swift

+
+    private func initStorage() throws {
+        if !FileManager.default.fileExists(atPath: dataDirectoryURL.path) {
+            try FileManager.default.createDirectory(at: dataDirectoryURL, withIntermediateDirectories: true)


We don’t want an error like BookmarksFaviconsFetcherError.failedToCreateDirectory(error)

We don't need it because this class is not initialized in BSK, but in client apps. The sole reason for having BookmarksFaviconsFetcherError is to be able to pass various errors to client apps via EventMapping< BookmarksFaviconsFetcherError>. For this function we don't need EventMapping because it's initialized directly in client apps that are expected to handle the error with a pixel as appropriate.

SabrinaTardio · 2023-11-28T16:54:59Z

Sources/Bookmarks/FaviconsFetcher/BookmarksFaviconsFetcherStateStore.swift

+
+    public func storeBookmarkIDs(_ ids: Set<String>) throws {
+        do {
+            try ids.joined(separator: ",").data(using: .utf8)?.write(to: missingIDsFileURL)


Would we want to differentiate failed to store from failed to convert?

I wouldn't worry about conversion to Data because UUIDs is something we should have control over, and converting to data would only return nil when a string can't be represented in UTF-8. We're even force unwrapping these calls in a bunch of places across the BSK codebase.

SabrinaTardio · 2023-11-29T08:57:29Z

Tests/BookmarksTests/FaviconsFetcher/BookmarksFaviconsFetcherTests.swift

+        await runAfterOperationsFinished {
+            let ids = (try? self.stateStore.getBookmarkIDs()) ?? []
+            XCTAssertTrue(ids.isEmpty)
+            XCTAssertTrue(MockBookmarksFaviconsFetcherEventMapper.errors.isEmpty)


This makes me think… should it be an error if empty?

This means that there are no bookmarks to handle which is an expected scenario for users that don't have bookmarks :)

ayoy · 2023-11-29T20:16:46Z

Thanks for the review @SabrinaTardio! For the prompt icons, the onboarding dialog uses the same icons. The settings option used different icons, but from what I can see, the newest designs get rid of the icons in settings altogether.

* main: bump privacy-dashboard to 3.0.0 (#549) Allow automated fetching of synced bookmarks' favicons (#564)

ayoy added 30 commits September 21, 2023 15:58

Add to-many relationship for favorites folders

df433e7

Update addToFavorites API usage

214da43

Update BookmarkTree and fix unit tests to support form factor specifi…

7d9e0ed

…c favorites

Make FavoritesConfiguration conform to Equatable and make its api public

4f4404e

Fix favorites syncable representation

1c45a40

Fix FavoritesConfiguration.folderUUIDs

0e83557

Fix favorites folders in MenuBookmarksViewModel

0e46410

Fix populating favorites in Sync response

4ea3455

Rename FavoritesConfiguration to FavoritesDisplayMode

2400000

Fix adding to favorites on iOS

d873593

Fix handling favorites in sync response handler

f33013c

Make favorites display mode public and modifiable in FavoriteListView…

bad2850

…Model

Make favorites display mode public and modifiable in MenuBookmarksVie…

bb08514

…wModel

Remove favoritesDisplayMode from initializers in FavoritesListViewMod…

547da01

…el and MenuBookmarksViewModel

Add favoritesDisplayMode to FavoritesListInteracting and MenuBookmark…

161088d

…sInteracting

Set renaming identifier for favoriteFolders

bb12fb7

Remove renamingIdentifier from BookmarkEntity

b4d5db3

Add a helper function to migrate to form factor specific favorites

21029fc

Add UserDefaultsSyncHandler for syncing settings stored in UserDefaults

e7db707

Add SettingSyncHandler class as a base class for all handlers for syn…

4643781

…cable settings

Add FavoritesDisplayMode.isDisplayAll

08b1419

Add BookmarkUtils.fetchFavoritesFoldersForUnfavoriting

b5ed45b

Move unfavoriting folders logic to BookmarkEntity

d27a0ff

Add BookmarkUtils.favoritesFoldersForUnfavoriting and keep BookmarkEn…

06f52b4

…tity unaware of display mode

Rename FavoritesPlatform with FavoritesFolderID and remove favorites …

20ac7bd

…folder IDs from Constants

Add display mode related API to BookmarkEntity extension

8f4f67f

Remove unneeded code

cb8ef49

Rename FavoritesFolderID all to unified

6bd32a3

Remove FavoriteListViewModel.favoritesFolder

ee5b87d

Restore favoriteFolder in FavoriteListViewModel

1c38686

ayoy added 7 commits November 10, 2023 17:48

Fix tests to compile

bdde10c

Improve error handling for BookmarksFaviconsFetcher

f5e869c

Extract favicon fetching and storing into a separate function

e428d5f

Add FaviconsFetchOperationTests

075bbee

BookmarkFaviconsFetcher* -> BookmarksFaviconsFetcher

451d2a5

Add tests for BookmarksFaviconsFetcher

4970150

Merge branch 'main' into dominik/sync-favicons-fetching

5eb8a71

ayoy self-assigned this Nov 15, 2023

ayoy added 3 commits November 15, 2023 16:00

Merge branch 'main' into dominik/sync-favicons-fetching

21e37ef

Fix SwiftLint violations

bf34a7a

Add documentation for bookmarks favicons fetcher

804ff87

ayoy marked this pull request as ready for review November 16, 2023 09:03

ayoy requested a review from bwaresiak November 16, 2023 09:06

ayoy assigned bwaresiak and unassigned ayoy Nov 16, 2023

ayoy added 5 commits November 17, 2023 09:48

Document LinkPresentation usage

d7955e9

Prioritize favorites and top level bookmarks when fetching favicons

f732dea

Refactor parts of FaviconsFetchOperation logic into BookmarkDomains

dae18a1

Fix swiftlint violations

cf12b59

Add unit tests for BookmarkDomains and fix implementation

c64925e

bwaresiak reviewed Nov 22, 2023

View reviewed changes

ayoy added 3 commits November 22, 2023 19:44

Update BookmarkUtils.fetchAllBookmarks to a better optimized Bookmark…

d7984d9

…Utils.fetchAllBookmarksUUIDs

Fetch also UUIDs in mapBookmarkDomainsToUUIDs

36d9a78

Merge branch 'main' into dominik/sync-favicons-fetching

2d1d73c

SabrinaTardio approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into dominik/sync-favicons-fetching

e76fef7

ayoy merged commit f1ae021 into main Nov 30, 2023
5 checks passed

ayoy deleted the dominik/sync-favicons-fetching branch November 30, 2023 07:19

samsymons added a commit that referenced this pull request Nov 30, 2023

Merge branch 'main' into sam/netp-waitlist-final-touches

b24d4b9

* main: bump privacy-dashboard to 3.0.0 (#549) Allow automated fetching of synced bookmarks' favicons (#564)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow automated fetching of synced bookmarks' favicons #564

Allow automated fetching of synced bookmarks' favicons #564

ayoy commented Nov 15, 2023 •

edited

Loading

bwaresiak Nov 22, 2023

ayoy Nov 22, 2023

ayoy Nov 22, 2023

bwaresiak Nov 22, 2023

ayoy Nov 22, 2023 •

edited

Loading

bwaresiak Nov 22, 2023

bwaresiak Nov 22, 2023

ayoy Nov 22, 2023

SabrinaTardio left a comment

SabrinaTardio Nov 28, 2023

ayoy Nov 29, 2023

SabrinaTardio Nov 28, 2023

ayoy Nov 29, 2023

SabrinaTardio Nov 28, 2023

ayoy Nov 29, 2023

SabrinaTardio Nov 29, 2023

ayoy Nov 29, 2023

ayoy commented Nov 29, 2023


		// MARK: - Overrides

		override var isAsynchronous: Bool { true }

Allow automated fetching of synced bookmarks' favicons #564

Allow automated fetching of synced bookmarks' favicons #564

Conversation

ayoy commented Nov 15, 2023 • edited Loading

Internal references:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayoy Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SabrinaTardio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayoy commented Nov 29, 2023

ayoy commented Nov 15, 2023 •

edited

Loading

ayoy Nov 22, 2023 •

edited

Loading