-
Notifications
You must be signed in to change notification settings - Fork 466
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
Fix StorageUI Bug and Update demo usage for SDWebImgae integration #842
Fix StorageUI Bug and Update demo usage for SDWebImgae integration #842
Conversation
1. The FIRStorage throw exception on passed url contains object path 2. The progress block may been called on main queue, should use another coder queue for decoding 3. The expectedSize should use URLResponse's info to check, but not the bodyLength
…, this can avoid blocking issue during massive image loading
samples/swift/FirebaseUI-demo-swift/Samples/StorageViewController.swift
Outdated
Show resolved
Hide resolved
… if queue is empty (to avoid too frequent callback), don't need to create AutoReleasePool as well
3393678
to
dcca215
Compare
}); | ||
}]; | ||
// Each download task use independent serial coder queue, to ensure callback in order during prorgessive decoding | ||
NSOperationQueue *coderQueue = [NSOperationQueue new]; |
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.
It looks like a new coderQueue
is allocated on each invocation of this method, but never retained anywhere. Is this intentional?
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.
It looks like a new coderQueue is allocated on each invocation of this method, but never retained anywhere.
It retained in the below, the dataWithMaxSize:completion:
block, here use coderQueue addOperationWithBlock:
. And also, the progressive one. The block keep a strong reference to capture Objecitve-C object by default (unless you mark __weak
).
Just use NSOperation to replace previous's GCD.
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.
The NSOperationQueue
lifetime is associated to the completion block, and the completion block lifetime is associated to FIRStorageDownloadTask
. This implements the design I want : Each download task use independent serial coder queue
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.
Sounds good.
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.
Or, if you think it's hard to find, we can change this into an associated object of FIRStorageDownloadTask
, like
@interface FIRStorageDownloadTask (StorageUI)
@property (nonatomic) NSOperationQueue *coderQueue;
@end
@implementation FIRStorageDownloadTask (StorageUI)
- (NSOperationQueue *)coderQueue {
NSOperationQueue *coderQueue = objc_getAsscoaitedObject(self, @selector(coderQueue));
if (!coderQueue) {
coderQueue = [NSOperationQueue new];
coderQueue.maxConcurrentOperationCount = 1;
objc_setAssociatedObject(self, @selector(coderQueue), coderQueue, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
}
return coderQueue;
}
@end
But actually, they have the same effect...
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 ok with the way the code is, just wanted to clarify 👍
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.
LGTM, thanks @dreampiggy!
If you find there are no extra review suggestion to check, feel free to merge. :) |
The Travis-CI failed bacause of CocoaPods repo update failed :( @morganchen12 Can you update the Gemfile and CI to use new CocoaPods version ? At least v1.8.0, which introduce the CDN trunk instead of Spec repo. Which don't need to wasting time to git clone the https://github.com/CocoaPods/Specs. It use a CDN for HTTP download, 10x faster. |
Updated gems in #844. |
The changes look fine to me, I'll take care of fixing the build. Thanks @dreampiggy! |
Hey there! So you want to contribute to FirebaseUI? Before you file this pull request, follow these steps:
Fix issues related StorageUI and SDWebImage, introduced in #841
1. The FIRStorage throw exception on passed url contains object path
2. The progress block may been called on main queue, should use another coder queue for decoding
3. The expectedSize should use URLResponse's info to check, but not the bodyLength
CC @morganchen12