Bug 170839 - Implement a way in WebItemProviderPasteboard to perform actions after file loading completes
Summary: Implement a way in WebItemProviderPasteboard to perform actions after file lo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-13 17:59 PDT by Wenson Hsieh
Modified: 2017-04-17 10:17 PDT (History)
5 users (show)

See Also:


Attachments
Part two (7.57 KB, patch)
2017-04-13 19:46 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (7.45 KB, patch)
2017-04-13 22:36 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (7.20 KB, patch)
2017-04-14 12:52 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (7.04 KB, patch)
2017-04-14 15:27 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (7.16 KB, patch)
2017-04-14 23:04 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-04-13 17:59:29 PDT
Work towards <rdar://problem/31286130>
Comment 1 Wenson Hsieh 2017-04-13 19:46:55 PDT
Created attachment 307074 [details]
Part two
Comment 2 Wenson Hsieh 2017-04-13 22:36:09 PDT
Created attachment 307093 [details]
Patch
Comment 3 Wenson Hsieh 2017-04-14 12:52:32 PDT
Created attachment 307137 [details]
Patch
Comment 4 Wenson Hsieh 2017-04-14 13:13:10 PDT
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.
Comment 5 Andy Estes 2017-04-14 13:23:02 PDT
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.
Comment 6 Wenson Hsieh 2017-04-14 15:27:07 PDT
Created attachment 307154 [details]
Patch
Comment 7 Tim Horton 2017-04-14 17:06:57 PDT
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.
Comment 8 Wenson Hsieh 2017-04-14 17:55:54 PDT
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.)
Comment 9 Wenson Hsieh 2017-04-14 23:04:27 PDT
Created attachment 307190 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-04-14 23:48:09 PDT
Comment on attachment 307190 [details]
Patch for landing

Clearing flags on attachment: 307190

Committed r215389: <http://trac.webkit.org/changeset/215389>
Comment 11 Wenson Hsieh 2017-04-15 02:24:57 PDT
Internal iOS build fix in <http://trac.webkit.org/changeset/215390>.