Refactors downloading and fixes download start time. #360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🚫 CI failed with log |
|
🚫 CI failed with log |
Adlai-Holler
approved these changes
May 23, 2017
|
|
||
| @interface PINURLSessionTaskObserver : NSObject | ||
| { | ||
| id selfCopy; |
There was a problem hiding this comment.
I guess this is a leftover?
|
|
||
| @property (atomic, assign) CFTimeInterval startTime; | ||
| @property (atomic, assign) CFTimeInterval endTime; | ||
| @property (nonatomic, weak, readonly) NSURLSessionTask *task; |
There was a problem hiding this comment.
Can probably remove this property unless it's used.
|
|
||
| @interface PINRemoteImageTask (Subclassing) | ||
|
|
||
| - (nonnull NSMutableDictionary *)l_callbackBlocks; |
There was a problem hiding this comment.
Might as well go in with NS_ASSUME_NONNULL here, and nullable PINResume *_Nullable * or whatever it is for the resume handler.
Source/Classes/PINProgressiveImage.m
Outdated
| NSAssert(_dataTask.PIN_startTime != 0, @"Start time needs to be set by now."); | ||
| CFTimeInterval endTime = CACurrentMediaTime(); | ||
| if (_dataTask.PIN_endTime != 0) { | ||
| endTime = _dataTask.PIN_endTime; |
There was a problem hiding this comment.
The logic here is a little bit hard to follow. Let's do something like CFTimeInterval endTime = (_dataTask.PIN_endTime ?: CACurrentMediaTime())
| @@ -1062,26 +1018,14 @@ - (void)cancelTaskWithUUID:(nonnull NSUUID *)UUID storeResumeData:(BOOL)storeRes | |||
| [strongSelf.canceledTasks addObject:UUID]; | |||
| } | |||
|
|
|||
| if ([taskToEvaluate cancelWithUUID:UUID manager:strongSelf]) { | |||
| if ([taskToEvaluate cancelWithUUID:UUID resume:storeResumeData ? &resume : nil]) { | |||
There was a problem hiding this comment.
Silly nit: this should be NULL since its type is id *
|
🚫 CI failed with log |
|
🚫 CI failed with log |
Download start time was being incorrectly computed since the timer was started when the image was added to the queue instead of when the image was actually started downloading. When attempting to address this, I decided to do some much needed cleanup and refactoring by moving much of the download work to PINRemoteImageDownloadTask
garrettmoon
force-pushed
the
refactorDownloadFixTiming
branch
from
1265ebe to
8891791
Compare
|
🚫 CI failed with log |
Generated by 🚫 Danger |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Download start time was being incorrectly computed since the timer
was started when the image was added to the queue instead of when the
image was actually started downloading.
When attempting to address this, I decided to do some much needed cleanup
and refactoring by moving much of the download work to PINRemoteImageDownloadTask