Summary: | Implement a way in WebItemProviderPasteboard to perform actions after file loading completes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Component: | WebKit2 | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aestes, bdakin, commit-queue, megan_gardner, thorton | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2017-04-13 17:59:29 PDT
Created attachment 307074 [details]
Part two
Created attachment 307093 [details]
Patch
Created attachment 307137 [details]
Patch
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 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. Created attachment 307154 [details]
Patch
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 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.) Created attachment 307190 [details]
Patch for landing
Comment on attachment 307190 [details] Patch for landing Clearing flags on attachment: 307190 Committed r215389: <http://trac.webkit.org/changeset/215389> Internal iOS build fix in <http://trac.webkit.org/changeset/215390>. |