Favicons #388
Favicons #388
Conversation
|
Hi Tom, a couple issues I've noticed so far:
I'm also wondering about the need to reload here - in Firefox, the favicon changes immediately. Some websites make use of real-time favicon updates - for example, Github shows the build status in the favicon, so that you can leave a tab in the background and see when the build finishes, and I remember seeing other websites use the favicon to show unread notifications. Was this discussed somewhere already?
https://favicon-test-61298f.netlify.app/favicon-test.html?icon=duck-10000px.png%3Fq%3D1
No normal website should run into this (the favicon in the demo site is 10000x10000px), but it seems like it could be misused pretty easily.
Examples: The other steps on your testing list all looked good. I'll do some testing with popular websites tomorrow and make sure there aren't any issues with specific sites. |
|
I ran through a list of the top 50 domains to check the icons with this PR. The tab icons look great! I didn't see any issues there. The homepage icons have a few issues compared to Safari - it seems that in some cases, Safari is able to find a high-resolution icon, while DDG is using a low-resolution one. Full results: https://docs.google.com/spreadsheets/d/10ps0nxrIg_-AuOshFrFQ5sSXXYucKNM3L_ndTiOOMjk/edit |
|
Awesome Ben! Thanks a lot, this is super helpful. I'm diving into it. |
…he fetching favicons from remote endpoinds. It helps to avoid synchronous reading of favicons from cache
…ning fixed for no favicons
|
@tomasstrba LGTM! Just one thing that stands out to me, when saving https://www.ebay.com/ the favorite icon looks really tiny, but when I checked on Safari it wasn't even there, FF does displays it, but not the favicon, so maybe it's not a real issue?
|
|
@Bunn, yeah you are right. That also the reason why the release of this feature was paused. I am holding the mid-mortem during the O-J today to find out appropriate next steps. |
|
@Bunn, we agreed to keep the homepage experience same as it is in the released app. I have already pushed the commit. There are also few remaining things to resolve related to the #380. From now on, I don't want to disturb you with this PR so next time this is ready for a review I will assign to Alex. Thanks a lot for your time and help here! 🙏 🙏 🙏 |
|
For sure! Feel free to ping me if you need help :) |
| URLSession.default.dataTask(with: faviconUrl) { data, _, error in | ||
| guard let data = data, error == nil else { | ||
| assertionFailure("Fetching failed") | ||
| return |
There was a problem hiding this comment.
You won't believe but I woke up in the middle of the night and realized this
| let group = DispatchGroup() | ||
| var favicons = [Favicon]() | ||
|
|
||
| faviconLinks.forEach { faviconLink in | ||
| guard let faviconUrl = URL(string: faviconLink.href) else { | ||
| return | ||
| } | ||
|
|
||
| group.enter() | ||
| URLSession.default.dataTask(with: faviconUrl) { data, _, error in | ||
| guard let data = data, error == nil else { | ||
| assertionFailure("Fetching failed") | ||
| return | ||
| } | ||
|
|
||
| let favicon = Favicon(identifier: UUID(), | ||
| url: faviconUrl, | ||
| image: NSImage(data: data), | ||
| relationString: faviconLink.rel, | ||
| documentUrl: documentUrl, | ||
| dateCreated: Date()) | ||
| DispatchQueue.main.async { | ||
| favicons.append(favicon) | ||
| group.leave() | ||
| } | ||
| mainQueueCompletion(favicons) | ||
| }.resume() | ||
| } | ||
|
|
||
| group.notify(queue: .main) { | ||
| completion(favicons) | ||
| } |
There was a problem hiding this comment.
I see no particular reasoning in synchronising fetched results using the Main Queue, although it shouldn't be a big deal speedwise, but still..
It used to be possible to asynchronously access array elements directly which creates no data race (favicons[idx] = favicon) because each element is pre-allocated, but now Thread Sanitizer gives a warning about favicons access data race. I've played with it a bit and found out that using withUnsafeMutablePointer(to: &favicons[idx]) solves the issue - this creates no data races and requires no extra threads synchronisation, see the suggested approach:
| let group = DispatchGroup() | |
| var favicons = [Favicon]() | |
| faviconLinks.forEach { faviconLink in | |
| guard let faviconUrl = URL(string: faviconLink.href) else { | |
| return | |
| } | |
| group.enter() | |
| URLSession.default.dataTask(with: faviconUrl) { data, _, error in | |
| guard let data = data, error == nil else { | |
| assertionFailure("Fetching failed") | |
| return | |
| } | |
| let favicon = Favicon(identifier: UUID(), | |
| url: faviconUrl, | |
| image: NSImage(data: data), | |
| relationString: faviconLink.rel, | |
| documentUrl: documentUrl, | |
| dateCreated: Date()) | |
| DispatchQueue.main.async { | |
| favicons.append(favicon) | |
| group.leave() | |
| } | |
| mainQueueCompletion(favicons) | |
| }.resume() | |
| } | |
| group.notify(queue: .main) { | |
| completion(favicons) | |
| } | |
| var favicons = [Favicon?].init(repeating: nil, count: faviconLinks.count) | |
| let group = DispatchGroup() | |
| faviconLinks.enumerated().forEach { idx, faviconLink in | |
| guard let faviconUrl = URL(string: faviconLink.href) else { | |
| return | |
| } | |
| group.enter() | |
| withUnsafeMutablePointer(to: &favicons[idx]) { favicon in | |
| URLSession.default.dataTask(with: faviconUrl) { data, _, error in | |
| defer { | |
| group.leave() | |
| } | |
| guard let data = data, error == nil else { | |
| assertionFailure("Fetching failed") | |
| return | |
| } | |
| favicon.pointee = Favicon(identifier: UUID(), | |
| url: faviconUrl, | |
| image: NSImage(data: data), | |
| relationString: faviconLink.rel, | |
| documentUrl: documentUrl, | |
| dateCreated: Date()) | |
| }.resume() | |
| } | |
| } | |
| group.notify(queue: .main) { | |
| completion(favicons.compactMap { $0 }) | |
| } |
There was a problem hiding this comment.
Thanks for this tip and also for finished suggestion. I have mixed feelings about this change because:
- The reason I implemented this safe is to eliminate any possibilities of data races we previosly had with favicons
- The suggestion speeds up adding to the array, unfortunately new array needs to be created on the completion (Hard to tell if we even speed things up overall)
- Users won't see any difference
- The readability of the suggestion is worse then the original code.
It was very interesting to see that there is a possibility to do it but I would stick with the original solution. What do you think?
|
I've checked the latest URLSession commit, Looks good to me overall, just see my comments above |
| group.enter() | ||
| URLSession.default.dataTask(with: faviconUrl) { data, _, error in | ||
| guard let data = data, error == nil else { | ||
| assertionFailure("Fetching failed") |
There was a problem hiding this comment.
Also I would expect it to be quite ok for a fetch request to fail, assertion here seems unneded
|
Note: I rolled back SecureVault exceptions since there was no reasonable way to do it without a major architecture change. It will be resolved as a follow-up task |
Task/Issue URL: https://app.asana.com/0/1206853951101015/1206853951101015 Autoconsent Release: https://github.com/duckduckgo/autoconsent/releases/tag/v10.3.0 ## Description Updates Autoconsent to version [v10.3.0](https://github.com/duckduckgo/autoconsent/releases/tag/v10.3.0). ### Autoconsent v10.3.0 release notes #### 🚀 Enhancement - DDG release automation [#389](duckduckgo/autoconsent#389) ([@muodov](https://github.com/muodov)) - Bump the dev-dependencies group with 4 updates [#390](duckduckgo/autoconsent#390) ([@dependabot[bot]](https://github.com/dependabot[bot])) #### 🐛 Bug Fix - Fix infinite reload for OneTrust sites [#393](duckduckgo/autoconsent#393) ([@muodov](https://github.com/muodov)) - Script to crawl page text content in multiple languages. [#386](duckduckgo/autoconsent#386) ([@sammacbeth](https://github.com/sammacbeth)) - Update Asana sync action [#388](duckduckgo/autoconsent#388) ([@sammacbeth](https://github.com/sammacbeth)) #### Authors: 3 - [@dependabot[bot]](https://github.com/dependabot[bot]) - Maxim Tsoy ([@muodov](https://github.com/muodov)) - Sam Macbeth ([@sammacbeth](https://github.com/sammacbeth)) ## Steps to test This release has been tested during Autoconsent development. You can check the release notes for more information. Co-authored-by: muodov <[email protected]>
Task/Issue URL: https://app.asana.com/0/72649045549333/1200560847598933/f
Tech Design URL: https://app.asana.com/0/481882893211075/1201323850042734/f
CC:
Description:
This project aim to solve following problems with favicons:
Steps to test this PR:
Test the overall experience
Test small favicons for the tab bar
Test medium favicons on the homepage
Test 16x16 favicon
Test different favicon for subdomains
Test permanent store of favicons
Test dark/aqua favicon alternatives
Test burning of favicons
Burning should remove favicon from cache and permanent cache if the appropriate host isn't in bookmarks or fireproofed domains.
This can be tested well with slow internet connection. Website has no favicon in cache if there is an empty space for few seconds in the tab tar
Note: Unit tests in progress
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM