Have ASNetworkImageNode report whether images were cached or not #639
Conversation
Adlai-Holler
requested review from
maicki,
garrettmoon,
appleguy and
nguyenhuy
| } else if (_delegateFlags.delegateDidLoadImage) { | ||
| ASDN::MutexUnlocker l(__instanceLock__); | ||
| [_delegate imageNode:self didLoadImage:result]; | ||
| } |
There was a problem hiding this comment.
We previously weren't calling out to the delegate at all in this synchronous fetch scenario. I think we should.
| } else { | ||
| dispatch_async(queue, work); | ||
| } | ||
| } |
There was a problem hiding this comment.
Added this method because I added another callback path and it felt silly to keep duplicating code. Sorry, dear reviewer!
| // feedback to the consumer about whether images are cached, we can't simply make the cache a no-op and | ||
| // check the cache as part of this download. | ||
| return [[self sharedPINRemoteImageManager] downloadImageWithURLs:URLs | ||
| options:PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache |
There was a problem hiding this comment.
Sadly I don't think we should skip the cache because the synchronous cache check only checks the memory cache not the disk cache.
There was a problem hiding this comment.
But we'll check the disk cache in cachedImageWithURL:callbackQueue:completion: so I think we're OK?
There was a problem hiding this comment.
I take this back, didn't see that the cache check above was implemented.
There was a problem hiding this comment.
For history, @Adlai-Holler and I discussed and we think this is safe. He pointed out the only case where this could cause an extra download is:
- request0 -> cache: miss
- request0 -> download: started
- request1 -> cache: miss
- request0 -> download: finished
- request1 -> download: started
This seems very unlikely in practice.
| // feedback to the consumer about whether images are cached, we can't simply make the cache a no-op and | ||
| // check the cache as part of this download. | ||
| return [[self sharedPINRemoteImageManager] downloadImageWithURLs:URLs | ||
| options:PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache |
There was a problem hiding this comment.
For history, @Adlai-Holler and I discussed and we think this is safe. He pointed out the only case where this could cause an extra download is:
- request0 -> cache: miss
- request0 -> download: started
- request1 -> cache: miss
- request0 -> download: finished
- request1 -> download: started
This seems very unlikely in practice.
| } else if (_delegateFlags.delegateDidLoadImage) { | ||
| ASDN::MutexUnlocker l(__instanceLock__); | ||
| [_delegate imageNode:self didLoadImage:result]; | ||
| } |
…tureGroup#639) * Have ASNetworkImageNode report whether images were cached or not. * Update changelog * Add fileURL case
This should help consumers who are interested in tracking their image cache performance.