[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]

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.