Bug 170839 – Implement a way in WebItemProviderPasteboard to perform actions after file loading completes

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards <rdar://problem/31286130>

[Wenson Hsieh]

Created attachment 307074 [details]
Part two

[Wenson Hsieh]

Created attachment 307093 [details]
Patch

[Wenson Hsieh]

Created attachment 307137 [details]
Patch

[Wenson Hsieh]

Comment on attachment 307137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307137&action=review

> Source/WebCore/ChangeLog:22
> +        (-[WebItemProviderPasteboard tryToLoadProvidedContentIntoFileURLs:]):

I renamed this method in the source, but didn't update the ChangeLogs -- I'll fix this.

[Andy Estes]

Comment on attachment 307137 [details]
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.

[Wenson Hsieh]

Created attachment 307154 [details]
Patch

[Tim Horton]

Comment on attachment 307154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307154&action=review

You should note all the reviewers in your something somewhere and figure out if it's enough.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:297
> +    auto fileLoadingGroup =  adoptOSObject(dispatch_group_create());

extra space after =

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:302
> +        dispatch_group_async(fileLoadingGroup.get(), dispatch_get_main_queue(), ^ {

You're gonna use some lambadas.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:309
> +                    [[NSFileManager defaultManager] removeItemAtURL:destinationURL.get() error:nil];

You're going to get rid of this.

[Wenson Hsieh]

Comment on attachment 307154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307154&action=review

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:297
>> +    auto fileLoadingGroup =  adoptOSObject(dispatch_group_create());
> 
> extra space after =

Good catch. Fixed.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:302
>> +        dispatch_group_async(fileLoadingGroup.get(), dispatch_get_main_queue(), ^ {
> 
> You're gonna use some lambadas.

Will do!

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:309
>> +                    [[NSFileManager defaultManager] removeItemAtURL:destinationURL.get() error:nil];
> 
> You're going to get rid of this.

Yep! (Dan Bernstein pointed out earlier that there is no need to try and clear out the destination URL since _webkit_createTemporaryDirectoryWithTemplatePrefix will either succeed and create a new directory, or fail and return nil, and this code the nil case by not adding a URL to the list of completed URLs.)

[Wenson Hsieh]

Created attachment 307190 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 307190 [details]
Patch for landing

Clearing flags on attachment: 307190

Committed r215389: <http://trac.webkit.org/changeset/215389>

[Wenson Hsieh]

Internal iOS build fix in <http://trac.webkit.org/changeset/215390>.