Ignore obsolete GIF requests.

// FREEBIE

Signal/src/ViewControllers/GifPicker/GifPickerCell.swift

@@ -13,19 +13,17 @@ class GifPickerCell: UICollectionViewCell {
         didSet {
             AssertIsOnMainThread()
 
-            ensureLoad()
+            ensureCellState()
         }
     }
 
     // Loading and playing GIFs is quite expensive (network, memory, cpu). 
     // Here's a bit of logic to not preload offscreen cells that are prefetched.
-    //
-    // TODO: Go a step farther and stop playback of cells that scroll offscreen.
-    var shouldLoad = false {
+    var isCellVisible = false {
         didSet {
             AssertIsOnMainThread()
 
-            ensureLoad()
+            ensureCellState()
         }
     }
 
@@ -53,7 +51,7 @@ class GifPickerCell: UICollectionViewCell {
         super.prepareForReuse()
 
         imageInfo = nil
-        shouldLoad = false
+        isCellVisible = false
         stillAsset = nil
         stillAssetRequest?.cancel()
         stillAssetRequest = nil
@@ -74,18 +72,22 @@ class GifPickerCell: UICollectionViewCell {
         fullAssetRequest = nil
     }
 
-    private func clearAssetRequest() {
+    private func clearAssetRequests() {
         clearStillAssetRequest()
         clearFullAssetRequest()
     }
 
-    private func ensureLoad() {
-        guard shouldLoad else {
-            clearAssetRequest()
+    private func ensureCellState() {
+        guard isCellVisible else {
+            // Cancel any outstanding requests.
+            clearAssetRequests()
+            // Clear image view so we don't animate offscreen GIFs.
+            imageView?.removeFromSuperview()
+            imageView = nil
             return
         }
         guard let imageInfo = imageInfo else {
-            clearAssetRequest()
+            clearAssetRequests()
             return
         }
         guard self.fullAsset == nil else {
@@ -95,41 +97,58 @@ class GifPickerCell: UICollectionViewCell {
         // It's critical that we carefully "pick" the best rendition to use.
         guard let fullRendition = imageInfo.pickGifRendition() else {
             Logger.warn("\(TAG) could not pick gif rendition: \(imageInfo.giphyId)")
-            clearAssetRequest()
+            clearAssetRequests()
             return
         }
         guard let stillRendition = imageInfo.pickStillRendition() else {
             Logger.warn("\(TAG) could not pick still rendition: \(imageInfo.giphyId)")
-            clearAssetRequest()
+            clearAssetRequests()
             return
         }
 
         if stillAsset == nil && fullAsset == nil && stillAssetRequest == nil {
             stillAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:stillRendition,
                                                                                 priority:.high,
-                                                                                success: { [weak self] asset in
+                                                                                success: { [weak self] assetRequest, asset in
                                                                                     guard let strongSelf = self else { return }
+                                                                                    if assetRequest != nil && assetRequest != strongSelf.stillAssetRequest {
+                                                                                        // Ignore obsolete requests.
+                                                                                        return
+                                                                                    }
                                                                                     strongSelf.clearStillAssetRequest()
                                                                                     strongSelf.stillAsset = asset
                                                                                     strongSelf.tryToDisplayAsset()
                 },
-                                                                                failure: { [weak self] in
+                                                                                failure: { [weak self] assetRequest in
                                                                                     guard let strongSelf = self else { return }
+                                                                                    if assetRequest != strongSelf.stillAssetRequest {
+                                                                                        // Ignore obsolete requests.
+                                                                                        return
+                                                                                    }
                                                                                     strongSelf.clearStillAssetRequest()
             })
         }
         if fullAsset == nil && fullAssetRequest == nil {
             fullAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:fullRendition,
                                                                                priority:.low,
-                                                                               success: { [weak self] asset in
+                                                                               success: { [weak self] assetRequest, asset in
                                                                                 guard let strongSelf = self else { return }
-                                                                                strongSelf.clearAssetRequest()
+                                                                                if assetRequest != nil && assetRequest != strongSelf.fullAssetRequest {
+                                                                                    // Ignore obsolete requests.
+                                                                                    return
+                                                                                }
+                                                                                // If we have the full asset, we don't need the still asset.
+                                                                                strongSelf.clearAssetRequests()
                                                                                 strongSelf.fullAsset = asset
                                                                                 strongSelf.tryToDisplayAsset()
                 },
-                                                                               failure: { [weak self] in
+                                                                               failure: { [weak self] assetRequest in
                                                                                 guard let strongSelf = self else { return }
-                                                                                strongSelf.clearAssetRequest()
+                                                                                if assetRequest != strongSelf.fullAssetRequest {
+                                                                                    // Ignore obsolete requests.
+                                                                                    return
+                                                                                }
+                                                                                strongSelf.clearFullAssetRequest()
             })
         }
     }

Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift

@@ -177,7 +177,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect
             return
         }
         // We only want to load the cells which are on-screen.
-        cell.shouldLoad = true
+        cell.isCellVisible = true
     }
 
     public func collectionView(_ collectionView: UICollectionView, didEndDisplaying cell: UICollectionViewCell, forItemAt indexPath: IndexPath) {
@@ -185,7 +185,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect
             owsFail("\(TAG) unexpected cell.")
             return
         }
-        cell.shouldLoad = false
+        cell.isCellVisible = false
     }
 
     // MARK: - Event Handlers

Signal/src/network/GifDownloader.swift

@@ -21,8 +21,8 @@ enum GiphyRequestPriority {
     // Exactly one of success or failure should be called once,
     // on the main thread _unless_ this request is cancelled before
     // the request succeeds or fails.
-    private var success: ((GiphyAsset) -> Void)
-    private var failure: (() -> Void)
+    private var success: ((GiphyAssetRequest?, GiphyAsset) -> Void)?
+    private var failure: ((GiphyAssetRequest) -> Void)?
 
     var wasCancelled = false
     // This property is an internal implementation detail of the download process.
@@ -30,8 +30,8 @@ enum GiphyRequestPriority {
 
     init(rendition: GiphyRendition,
          priority: GiphyRequestPriority,
-         success:@escaping ((GiphyAsset) -> Void),
-         failure:@escaping (() -> Void)
+         success:@escaping ((GiphyAssetRequest?, GiphyAsset) -> Void),
+         failure:@escaping ((GiphyAssetRequest) -> Void)
         ) {
         self.rendition = rendition
         self.priority = priority
@@ -51,17 +51,14 @@ enum GiphyRequestPriority {
     private func clearCallbacks() {
         AssertIsOnMainThread()
 
-        // Replace success and failure with no-ops.
-        success = { _ in
-        }
-        failure = {
-        }
+        success = nil
+        failure = nil
     }
 
     public func requestDidSucceed(asset: GiphyAsset) {
         AssertIsOnMainThread()
 
-        success(asset)
+        success?(self, asset)
 
         // Only one of the callbacks should be called, and only once.
         clearCallbacks()
@@ -70,7 +67,7 @@ enum GiphyRequestPriority {
     public func requestDidFail() {
         AssertIsOnMainThread()
 
-        failure()
+        failure?(self)
 
         // Only one of the callbacks should be called, and only once.
         clearCallbacks()
@@ -212,17 +209,19 @@ extension URLSessionTask {
     private let kMaxAssetRequestCount = 3
     private var activeAssetRequests = Set<GiphyAssetRequest>()
 
-    // The success and failure handlers are always called on main queue.
-    // The success handler may be called synchronously on cache hit.
+    // The success and failure callbacks are always called on main queue.
+    //
+    // The success callbacks may be called synchronously on cache hit, in 
+    // which case the GiphyAssetRequest parameter will be nil.
     public func requestAsset(rendition: GiphyRendition,
                              priority: GiphyRequestPriority,
-                             success:@escaping ((GiphyAsset) -> Void),
-                             failure:@escaping (() -> Void)) -> GiphyAssetRequest? {
+                             success:@escaping ((GiphyAssetRequest?, GiphyAsset) -> Void),
+                             failure:@escaping ((GiphyAssetRequest) -> Void)) -> GiphyAssetRequest? {
         AssertIsOnMainThread()
 
         if let asset = assetMap.get(key:rendition.url) {
             // Synchronous cache hit.
-            success(asset)
+            success(nil, asset)
             return nil
         }