Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix FXIOS-9830 Do not duplicate favicon URL and image requests on UITableViewCell reload [Favicon Refactor] #21587
Bugfix FXIOS-9830 Do not duplicate favicon URL and image requests on UITableViewCell reload [Favicon Refactor] #21587
Changes from 22 commits
773d10a
c1fb213
ed573a8
8647976
dbbb81d
726c0ec
902f5aa
406533a
2d17bd8
b2b0699
e467a27
bb173eb
00a714d
02056a7
967ab07
308bc87
f9ceed9
2b0baae
6a69dd9
027dfe0
9aeadad
212eb84
d5043ee
872cb5b
f70af63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my only concern with this is the introduction of this global state; it's somewhat obscured that all of these instances will be potentially checking against the same shared request queue, even if they are initialized with different url handlers or image handlers. Right now I don't think that matters much in practice, and it's hard to imagine how it would realistically cause problems but it might be something nice for us to revisit at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally understand. Ideally this would all be refactored to be using a single instance of
DefaultSiteImageHandler
passed in to each cell, but this would entail a big refactor...I'm ok with leaving the current solution in as well, as long as we can get my new unit test to pass reliably for it. Since that (I believe) mimics the current state of affairs with multiple instances of DefaultSiteImageHandler being created for a single cell/site URL.
I'll experiment with it a bit this afternoon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went back and forth on this a bunch when originally building it if this should be a singleton or not. In the end we opted to go with the simpler solution and live with the fact there would occasionally be the odd unnecessary request but obviously that has turned out to be a worse problem than we anticipated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too worried about the global state from an app stability perspective because it's private and it's within a pretty contained submodule but it's a little concerning from a test perspective because of the possibility of one test colliding with another. It's not a major concern tho and seems like a reasonable solution that limits the scope of the change. Just worth keeping an eye on if we start getting flaky tests in this area in the future we might want to revisit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @OrlaM! That makes sense. This is definitely something to keep an eye on in the future... I was browsing Sentry to see if I could find any
EXC_BAD_ACCESS
type crashes related to favicons and saw this one. Made me wonder if it was something related to this recreation of theSiteImageHandler
.