[webkit-reviews] review granted: [Bug 170839] Implement a way in WebItemProviderPasteboard to perform actions after file loading completes : [Attachment 307137] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 14 13:23:02 PDT 2017
Andy Estes <aestes at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 170839: Implement a way in WebItemProviderPasteboard to perform actions
after file loading completes
https://bugs.webkit.org/show_bug.cgi?id=170839
Attachment 307137: Patch
https://bugs.webkit.org/attachment.cgi?id=307137&action=review
--- Comment #5 from Andy Estes <aestes at apple.com> ---
Comment on attachment 307137
--> https://bugs.webkit.org/attachment.cgi?id=307137
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307137&action=review
> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:34
> +typedef void (^WebItemProviderFileLoadBlock)(NSArray <NSURL *> *);
I think our style is to not include a space between NSArray and <.
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:267
> + NSString *temporaryFilename = suggestedName ?: (NSString
*)String::number(cryptographicallyRandomNumber());
If each temporary file is stored inside a unique directory name, and a
suggested name isn't provided, do we need to use a cryptographically random
name for the temporary file? Could it just be a fixed name?
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:280
> + if (!UTTypeConformsTo((__bridge CFStringRef)identifier,
kUTTypeContent))
Is a bridging cast needed here?
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:298
> + dispatch_group_t fileLoadingGroup = dispatch_group_create();
Can we use OSObjectPtr here?
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:303
> + dispatch_group_async(fileLoadingGroup, dispatch_get_main_queue(),
^() {
No need to specify () in a no-argument block.
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:313
> + [[NSFileManager defaultManager] linkItemAtURL:url
toURL:destinationURL.get() error:&hardLinkError];
> + if (!hardLinkError)
It's idiomatic to check the return value of -linkItemAtURL:toURL:error: instead
of checking for a non-nil error. We guarantee that error will be non-nil when
an error occurs, but not that it'll be nil when no error occurs.
> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:321
> + dispatch_group_notify(fileLoadingGroup, dispatch_get_main_queue(), ^() {
No need to specify () in a no-argument block.
More information about the webkit-reviews
mailing list