Clean up ahead of PR.

// FREEBIE

Signal/src/ViewControllers/GifPicker/GifPickerCell.swift

@@ -105,7 +105,7 @@ class GifPickerCell: UICollectionViewCell {
         }
 
         if stillAsset == nil && fullAsset == nil && stillAssetRequest == nil {
-            stillAssetRequest = GifDownloader.sharedInstance.downloadAssetAsync(rendition:stillRendition,
+            stillAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:stillRendition,
                                                                                 priority:.high,
                                                                                 success: { [weak self] asset in
                                                                                     guard let strongSelf = self else { return }
@@ -119,7 +119,7 @@ class GifPickerCell: UICollectionViewCell {
             })
         }
         if fullAsset == nil && fullAssetRequest == nil {
-            fullAssetRequest = GifDownloader.sharedInstance.downloadAssetAsync(rendition:fullRendition,
+            fullAssetRequest = GifDownloader.sharedInstance.requestAsset(rendition:fullRendition,
                                                                                priority:.low,
                                                                                success: { [weak self] asset in
                                                                                 guard let strongSelf = self else { return }

Signal/src/network/GifDownloader.swift

@@ -5,6 +5,7 @@
 import Foundation
 import ObjectiveC
 
+// Stills should be loaded before full GIFs.
 enum GiphyRequestPriority {
     case low, high
 }
@@ -148,16 +149,16 @@ class LRUCache<KeyType: Hashable & Equatable, ValueType> {
     }
 }
 
-private var URLSessionTask_GiphyAssetRequest: UInt8 = 0
+private var URLSessionTaskGiphyAssetRequest: UInt8 = 0
 
 // This extension is used to punch an asset request onto a download task.
 extension URLSessionTask {
     var assetRequest: GiphyAssetRequest {
         get {
-            return objc_getAssociatedObject(self, &URLSessionTask_GiphyAssetRequest) as! GiphyAssetRequest
+            return objc_getAssociatedObject(self, &URLSessionTaskGiphyAssetRequest) as! GiphyAssetRequest
         }
         set {
-            objc_setAssociatedObject(self, &URLSessionTask_GiphyAssetRequest, newValue, objc_AssociationPolicy.OBJC_ASSOCIATION_RETAIN_NONATOMIC)
+            objc_setAssociatedObject(self, &URLSessionTaskGiphyAssetRequest, newValue, objc_AssociationPolicy.OBJC_ASSOCIATION_RETAIN_NONATOMIC)
         }
     }
 }
@@ -197,11 +198,11 @@ extension URLSessionTask {
         return session
     }
 
-    // 100 entries of which at least half will probably be stills.  
-    // Actual animated GIFs will usually be less than 3 MB so the 
+    // 100 entries of which at least half will probably be stills.
+    // Actual animated GIFs will usually be less than 3 MB so the
     // max size of the cache on disk should be ~150 MB.  Bear in mind
     // that assets are not always deleted on disk as soon as they are
-    // evacuated from the cache; if a cache consumer (e.g. view) is 
+    // evacuated from the cache; if a cache consumer (e.g. view) is
     // still using the asset, the asset won't be deleted on disk until
     // it is no longer in use.
     private var assetMap = LRUCache<NSURL, GiphyAsset>(maxSize:100)
@@ -212,24 +213,28 @@ extension URLSessionTask {
     private var activeAssetRequests = Set<GiphyAssetRequest>()
 
     // The success and failure handlers are always called on main queue.
-    // The success and failure handlers may be called synchronously on cache hit.
-    public func downloadAssetAsync(rendition: GiphyRendition,
-                                   priority: GiphyRequestPriority,
-                              success:@escaping ((GiphyAsset) -> Void),
-                              failure:@escaping (() -> Void)) -> GiphyAssetRequest? {
+    // The success handler may be called synchronously on cache hit.
+    public func requestAsset(rendition: GiphyRendition,
+                             priority: GiphyRequestPriority,
+                             success:@escaping ((GiphyAsset) -> Void),
+                             failure:@escaping (() -> Void)) -> GiphyAssetRequest? {
         AssertIsOnMainThread()
 
         if let asset = assetMap.get(key:rendition.url) {
+            // Synchronous cache hit.
             success(asset)
             return nil
         }
 
+        // Cache miss.
+        //
+        // Asset requests are done queued and performed asynchronously.
         let assetRequest = GiphyAssetRequest(rendition:rendition,
                                              priority:priority,
-                                             success : success,
-                                             failure : failure)
+                                             success:success,
+                                             failure:failure)
         assetRequestQueue.append(assetRequest)
-        downloadIfNecessary()
+        startRequestIfNecessary()
         return assetRequest
     }
 
@@ -238,7 +243,7 @@ extension URLSessionTask {
             self.assetMap.set(key:assetRequest.rendition.url, value:asset)
             self.activeAssetRequests.remove(assetRequest)
             assetRequest.requestDidSucceed(asset:asset)
-            self.downloadIfNecessary()
+            self.startRequestIfNecessary()
         }
     }
 
@@ -246,11 +251,11 @@ extension URLSessionTask {
         DispatchQueue.main.async {
             self.activeAssetRequests.remove(assetRequest)
             assetRequest.requestDidFail()
-            self.downloadIfNecessary()
+            self.startRequestIfNecessary()
         }
     }
 
-    private func downloadIfNecessary() {
+    private func startRequestIfNecessary() {
         AssertIsOnMainThread()
 
         DispatchQueue.main.async {
@@ -262,7 +267,7 @@ extension URLSessionTask {
             }
             guard !assetRequest.wasCancelled else {
                 // Discard the cancelled asset request and try again.
-                self.downloadIfNecessary()
+                self.startRequestIfNecessary()
                 return
             }
             self.activeAssetRequests.insert(assetRequest)
@@ -276,11 +281,12 @@ extension URLSessionTask {
             }
 
             guard let downloadSession = self.giphyDownloadSession() else {
-                Logger.error("\(GifDownloader.TAG) Couldn't create session manager.")
+                owsFail("\(GifDownloader.TAG) Couldn't create session manager.")
                 self.assetRequestDidFail(assetRequest:assetRequest)
                 return
             }
 
+            // Start a download task.
             let task = downloadSession.downloadTask(with:assetRequest.rendition.url as URL)
             task.assetRequest = assetRequest
             task.resume()
@@ -290,6 +296,8 @@ extension URLSessionTask {
     private func popNextAssetRequest() -> GiphyAssetRequest? {
         AssertIsOnMainThread()
 
+        // Prefer the first "high" priority request, 
+        // fall back to the first "low" priority request.
         for priority in [GiphyRequestPriority.high, GiphyRequestPriority.low] {
             for (assetRequestIndex, assetRequest) in assetRequestQueue.enumerated() {
                 if assetRequest.priority == priority {
@@ -354,6 +362,8 @@ extension URLSessionTask {
             return
         }
 
+        // We write assets to the temporary directory so that iOS can clean them up.
+        // We try to eagerly clean up these assets when they are no longer in use.
         let dirPath = NSTemporaryDirectory()
         let fileExtension = assetRequest.rendition.fileExtension()
         let fileName = (NSUUID().uuidString as NSString).appendingPathExtension(fileExtension)!

Signal/src/network/GifManager.swift

@@ -10,6 +10,11 @@ enum GiphyFormat {
     case gif, mp4, jpg
 }
 
+// Represents a "rendition" of a GIF.
+// Giphy offers a plethora of renditions for each image.
+// They vary in content size (i.e. width,  height), 
+// format (.jpg, .gif, .mp4, webp, etc.),
+// quality, etc.
 @objc class GiphyRendition: NSObject {
     let format: GiphyFormat
     let name: String
@@ -59,9 +64,12 @@ enum GiphyFormat {
     }
 }
 
+// Represents a single Giphy image.
 @objc class GiphyImageInfo: NSObject {
     let giphyId: String
     let renditions: [GiphyRendition]
+    // We special-case the "original" rendition because it is the 
+    // source of truth for the aspect ratio of the image.
     let originalRendition: GiphyRendition
 
     init(giphyId: String,
@@ -89,6 +97,7 @@ enum GiphyFormat {
     }
 
     public func pickStillRendition() -> GiphyRendition? {
+        // Stills are just temporary placeholders, so use the smallest still possible.
         return pickRendition(isStill:true, pickingStrategy:.smallerIsBetter, maxFileSize:kMaxFileSize)
     }
 
@@ -101,22 +110,32 @@ enum GiphyFormat {
         if let rendition = pickRendition(isStill:false, pickingStrategy:.smallerIsBetter, maxFileSize:kMaxFileSize * 2) {
             return rendition
         }
-        // ...and relax even more.
+        // ...and relax even more until we find an animated rendition.
         return pickRendition(isStill:false, pickingStrategy:.smallerIsBetter, maxFileSize:kMaxFileSize * 3)
     }
 
+    // Picking a rendition must be done very carefully.
+    //
+    // * We want to avoid incomplete renditions.
+    // * We want to pick a rendition of "just good enough" quality.
     private func pickRendition(isStill: Bool, pickingStrategy: PickingStrategy, maxFileSize: UInt) -> GiphyRendition? {
         var bestRendition: GiphyRendition?
 
         for rendition in renditions {
             if isStill {
+                // Accept GIF or JPEG stills.  In practice we'll
+                // usually select a JPEG since they'll be smaller.
                 guard [.gif, .jpg].contains(rendition.format) else {
                     continue
                 }
+                // Only consider still renditions.
                 guard rendition.name.hasSuffix("_still") else {
                         continue
                 }
                 // Accept renditions without a valid file size.
+                //
+                // Don't worry about max content size; still images are tiny in comparison
+                // with animated renditions.
                 guard rendition.width >= kMinDimension &&
                     rendition.height >= kMinDimension &&
                     rendition.fileSize <= maxFileSize
@@ -124,12 +143,15 @@ enum GiphyFormat {
                         continue
                 }
             } else {
+                // Only use GIFs for animated renditions.
                 guard rendition.format == .gif else {
                     continue
                 }
+                // Ignore stills.
                 guard !rendition.name.hasSuffix("_still") else {
                         continue
                 }
+                // Ignore "downsampled" renditions which skip frames, etc.
                 guard !rendition.name.hasSuffix("_downsampled") else {
                         continue
                 }
@@ -145,11 +167,21 @@ enum GiphyFormat {
             }
 
             if let currentBestRendition = bestRendition {
-                if pickingStrategy == .smallerIsBetter {
+                if rendition.width == currentBestRendition.width &&
+                    rendition.fileSize > 0 &&
+                    currentBestRendition.fileSize > 0 &&
+                    rendition.fileSize < currentBestRendition.fileSize {
+                    // If two renditions have the same content size, prefer
+                    // the rendition with the smaller file size, e.g.
+                    // prefer JPEG over GIF for stills.
+                    bestRendition = rendition
+                } else if pickingStrategy == .smallerIsBetter {
+                    // "Smaller is better"
                     if rendition.width < currentBestRendition.width {
                         bestRendition = rendition
                     }
                 } else {
+                    // "Larger is better"
                     if rendition.width > currentBestRendition.width {
                         bestRendition = rendition
                     }
@@ -215,7 +247,9 @@ enum GiphyFormat {
             return
         }
 
-        // TODO: Should we use a separate API key?
+        // This is the Signal Android API key.
+        //
+        // TODO: Should Signal iOS use a separate API key?
         let kGiphyApiKey = "3o6ZsYH6U6Eri53TXy"
         let kGiphyPageSize = 200
         // TODO:
@@ -320,6 +354,8 @@ enum GiphyFormat {
     }
 
     // Giphy API results are often incomplete or malformed, so we need to be defensive.
+    //
+    // We should discard renditions which are missing or have invalid properties.
     private func parseGiphyRendition(renditionName: String,
                                      renditionDict: [String:Any]) -> GiphyRendition? {
         guard let width = parsePositiveUInt(dict:renditionDict, key:"width", typeName:"rendition") else {