Bugfix FXIOS-9427 Fixes for favicon issues [WIP]

[mattreaganmozilla]

📜 Tickets

Jira ticket
Github issue

💡 Description

Early work to fix a couple problems with our favicon fetching. I'll be adding some notes inline, please also see forthcoming comments in PR diff.

This should resolve at least a couple problems with favicons:

• The client is sometimes making redundant requests both for the website HTML (to scrape for the favicon path) and also the favicons themselves
• The client often attempts to request the favicon from an incorrect directory (the full website path with favicon appended)
• Our metadata parser is also returning what appears to be incorrect favicon URLs separately, and unfortunately the related JS repo is archived and cannot be fixed directly

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[mattreaganmozilla]

@OrlaM @nbhasin2 This isn't particularly elegant but it should fix the redundant requests both for the site HTML and favicons and is the simplest fix I could find that also didn't cause any other obvious regressions. If anyone has suggestions on a better solution, however, please LMK. Happy to sync up or chat further.

[OrlaM]

Is the source of the problem that one SiteImageView is trying to request the favicon multiple times or that multiple SiteImageViews are fetching the same favicon?
If the former then at what point is the current solution that is supposed to catch this failing? It's supposed to check against the currentURLString before it even gets to the image handler.
If it's the later then this fix won't help because SiteImageHandler is created new for each SiteImageView.

[mattreaganmozilla]

If the former then at what point is the current solution that is supposed to catch this failing? It's supposed to check against the currentURLString before it even gets to the image handler.

So this is a little bit more complicated than it seems, there is some code that is calling manuallySetImage repeatedly in the midst of also repeatedly updating the favicon to the same/identical URL. This results in the currentURLString getting reset basically so while it still serves a valid purpose, unfortunately the way that we're (over-)refreshing things still breaks it and causes redundant requests to be fired off.

If it's the later then this fix won't help because SiteImageHandler is created new for each SiteImageView.

I've been testing this fairly exhaustively and if you put a breakpoint or log in the code which kicks off the network requests for the site HTML or the favicons, that code is called multiple times when first visiting a website, but with this change in place it is correctly only hit once (and subsequent requests wait until the original one is completed). LMK if you're seeing something different however and/or if you have steps that are still replicating the issue.

[mattreaganmozilla]

Note: this change was suggested by @issammani (ty), and is part of the fix for the incorrect favicon URLs, which are actually coming from two different places, one of which being the JS metadata parser.

[mobiletest-ci-bot]

	Messages
📖	Edited 6 files
📖	Created 0 files

Generated by 🚫 Danger Swift against 0ec1d78

[mattreaganmozilla]

(Note: the failing unit test testCompileListsNotInStore_callsCompletionHandlerSuccessfully is a known issue and being investigated separately)

[mattreaganmozilla]

I've done some additional regression/smoke testing on this branch and couldn't identify any issues so planning to merge shortly. But if anyone has follow-up feedback LMK and I'm happy to revisit.