Assorted fixes and updates to HTML New Tab Page #3748
Conversation
| return nil | ||
| } | ||
| return .init(syncService: syncService, syncBookmarksAdapter: syncBookmarksAdapter) | ||
| }() | ||
| return ContentBlocking.shared.tld.eTLDplus1(domain)?.dropping(prefix: Const.wwwPrefix) |
There was a problem hiding this comment.
You could just initialise TLD directly in this class rather then using the instance in ContentBlocking
There was a problem hiding this comment.
That would mean adding TLD to Bookmark class which didn't sound good to me. The alternative I considered was the add TLD computing functionality in The NTP Favorites model, but that would be a much bigger change. The impact of this is very little so I decided to go with a simple solution like this, especially that we use exactly the same approach in PinnedTabInnerView and LetterIconView.
|
|
||
| private lazy var faviconsFetcherOnboarding: FaviconsFetcherOnboarding? = { | ||
| guard let syncService = NSApp.delegateTyped.syncService, let syncBookmarksAdapter = NSApp.delegateTyped.syncDataProviders?.bookmarksAdapter else { | ||
| assertionFailure("SyncService and/or SyncBookmarksAdapter is nil") | ||
| return nil | ||
| } | ||
| return .init(syncService: syncService, syncBookmarksAdapter: syncBookmarksAdapter) | ||
| }() |
There was a problem hiding this comment.
Probably I am missing some context, plus some of the favicon logic here was here from before… But in general seems a little strange for this and the favicon logic to live in DuckSchemeHandler and not in Bookmark or somewhere more specific
There was a problem hiding this comment.
Again, this isn't different from current usage of the favicons fetcher onboarding. We need to access Sync Service to check whether we should present it. For old NTP this was done in HomePageViewController, and for html NTP it's in the scheme handler because there's where favicons are requested and where we have to respond to favicons being missing.
Task/Issue URL: https://app.asana.com/0/72649045549333/1209186302756177/f
Description:
This change contains updates to HTML New Tab Page: fixed displaying favicons fetcher
onboarding dialog, added user image background thumbnails context menu
and updated favicon placeholder to 2 letters.
Steps to test this PR:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation