firefox-ios icon indicating copy to clipboard operation
firefox-ios copied to clipboard

Bugfix FXIOS-9830 Do not duplicate favicon URL and image requests on UITableViewCell reload

Open ih-codes opened this issue 1 year ago • 1 comments

:scroll: Tickets

Jira ticket Github issue

:bulb: Description

Note I have the merge destination currently as my other favicon refactor branch so the changes can be more clearly reviewed. But merge in FXIOS-9667 fix first.


Revises the solution for this FXIOS-9427 bugfix as the solution did not prevent duplicate network calls when loading top site tiles on the Home screen (if the DefaultSiteImageHandler was destroyed on cell reload, for example).

Ideally the fix would be a larger architectural change, but this is a stop gap measure for now. But the TopSiteItemCell really should share a common reference to oneDefaultSiteImageHandler so we can better orchestrate multiple requests to the same resource (i.e. queue, throttle, etc.).

As it stands, the multiple reloads of things on the Home tab cause the cells to reload multiple times and trigger multiple fetches of the same favicon URL and image. If a new image is not yet in the cache, for example, that might trigger 3 network calls to download the image.

:pencil: Checklist

You have to check all boxes before merging

  • [x] Filled in the above information (tickets numbers and description of your work)
  • [x] Updated the PR name to follow our PR naming guidelines
  • [x] 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)

ih-codes avatar Aug 22 '24 15:08 ih-codes

This is ready for another look whenever you have time @mattreaganmozilla (no rush).

I removed the actor to instead annotate the method in question with @MainThread. Hopefully that resolves your concerns about code smell.

I saw some crashes while running the code in the sim, but that was after I removed actor and before I added the @MainActor annotation. 🤞 🙏

I also added a unit test which replicates the behaviour of TopSiteItemCells reloading on the home screen (which was causing multiple requests to be made to new instances of SiteImageHandlers). Overall, I think this code is an improvement simply because I can get my test to pass reliably after hundreds of iterations, whereas on main I get crashes and duplicated network calls. 🤔

ih-codes avatar Aug 23 '24 22:08 ih-codes

This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏

mergify[bot] avatar Aug 29 '24 15:08 mergify[bot]

Messages
:book: Edited 3 files
:book: Created 0 files

Generated by :no_entry_sign: Danger Swift against f70af638ccf57f5eb5dd449215ee21fbdd34ca68

mobiletest-ci-bot avatar Aug 29 '24 15:08 mobiletest-ci-bot