Lost in a Quagmire of FavIcon code

[gutley]

HI, So I've been updating my fork with the changes from 110.2 (I was previously on 109). In my fork I sometimes need to replace the URL in the current tab with a local file url of a file in the app bundle to show that the site is blocked.

In 109 and earlier I was able to easily replace the FavIcon shown in the tab bar with my own one by modifying UIImageView.setImageAndBackground(forIcon icon: Favicon?, website: URL?, completion: @escaping () -> Void) similar to this:

    func setImageAndBackground(forIcon icon: Favicon?, website: URL?, completion: @escaping () -> Void) {
   ......
        backgroundColor = nil
        let defaults = fallbackFavicon(forUrl: website)

        if let url = website, url.scheme == "file" {
            // It's our block page, so grab our favicon.
            if let ourFavicon = UIImage(contentsOfFile: URL.ourBlockPageFavicon.path) {
                self.image = ourFavicon
                finish(bgColor: nil)
                return
            }
            // Otherwise leave it to the default handling below
        }

        if let url = website, let bundledIcon = FaviconFetcher.getBundledIcon(forUrl: url) {
  ........

With 110, I can't find a simple place to do this, I've tried going down the rabbit whole of the new async FavIcon handling but without success as it's so hard to follow along. As such, the icon used for my block page is now this meaningless "F" symbol instead:

I am guessing that somewhere there is a fallback to just using the first character of the url ('f' for my File URL) and generating an image from that.

Can someone suggest the best place to modify to get my own icon back in the tab.

[lmarceau]

Hello! Indeed, there is a fallback whenever we can't fetch favicons for a website and we take the first letter to generate a default favicon. This is done in class LetterImageGenerator. I guess what you want is to have a check before we enter in this LetterImageGenerator, so that would be in the ImageHandler (default implementation with DefaultImageHandler). Hope this helps for your use case!

[gutley]

Thanks @lmarceau

In the end I think I have a better solution. This is based on the fact that none of that favicon loading code seems to have been designed with the idea that it might be being asked to deal with a file-based url instead of http[s], so its best to avoid it as much as possible.

In DefaultImageHandler.fetchFavIcon:

    func fetchFavicon(site: SiteImageModel) async -> UIImage {
        do {
            // If its a file URL, see if there is a favicon file sitting next to the original
            if let siteURL = site.siteURL,
               siteURL.scheme?.lowercased() == "file",
               let favIconURL = URL(string: "favicon.ico", relativeTo: siteURL),
               let image = UIImage(contentsOfFile: favIconURL.path) {
                return image
            }

            return try bundleImageFetcher.getImageFromBundle(domain: site.domain)
        } catch {
            return await fetchFaviconFromCache(site: site)
        }
    }

It just sees if there is a favicon.ico in the same directory as the page and uses that, short circuiting the rest of the code. If there isn't then it lets it fall through to the existing code.

But as I said that existing code currently can never cope well with file urls so will always fall back to the letter generator.

FYI In case you are wondering just how wonky the current code goes with file urls.

1. site.domains contains 2 entries, the entire url string itself, and its full path. In my case (snipped for brevity):
   file:///<path to bundle>/Client.app/Blocked/blockpage.html
/<path to bundle>/Client.app/Blocked/blockpage.html

This is definitely not something the rest of the code is expecting.

2. DefaultFaviconURLFetcher.processHTMLDocument's fall back gets it wrong:
   i.e this code:

        // Fallback to the favicon at the root of the domain
        // This is a fall back because it's generally low res
        if let faviconURL = URL(string: siteURL.absoluteString + "/favicon.ico") {
            return faviconURL
        }

generates:
file:///<path to bundle>/Client.app/Blocked/blockpage.html/favicon.ico

I did experiment with just fixing that last one for file URLs and hoping it would suffice to let the rest of the code deal with at least having a valid file-based site.faviconURL, but it didn't. For my needs I didn't need to investigate further.

I don't know if you want me to do a PR with my current fix in it or just leave it to my fork going forwards. I think I favour the latter with maybe one of you guys eventually improving the code more thoroughly should you ever decide to better cope with file urls.

Ideally it would be nice if there were a couple of delegate methods that BrowserKit would call to allow forkers to attempt their own image generation. One that gets called right at the start of fetchFavicon and one before it falls back to using the letter generator.