File promise during drag is not supported when WebKit is the drop target. We should add this, since Photos uses File Promise for drag and drop. rdar://problem/19595567
Created attachment 295733 [details] Patch
Attachment 295733 [details] did not pass style-queue: ERROR: Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3635: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3636: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3637: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3639: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3650: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.h:585: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit/mac/WebView/WebView.mm:6492: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit/mac/WebView/WebView.mm:6494: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WebKit/mac/WebView/WebView.mm:6496: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WebKit/mac/WebView/WebView.mm:6506: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/WebKit/mac/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 13 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 295733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295733&action=review > Source/WebCore/platform/mac/DragDataMac.mm:148 > + return numberOfFiles() == 1; Why does having one file mean that it is a promise? Couldn't you have one file before?
Comment on attachment 295733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295733&action=review > Source/WebCore/ChangeLog:10 > + Adds the support for handling File Promise type during > + drag. Can you flesh out this ChangeLog (and the others)? Please include information about why this is 10.12 only. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:1262 > dragData = DragData(pasteboardName, clientPosition, globalPosition, draggingSourceOperationMask, applicationFlags); > +#if PLATFORM(MAC) > + dragData.setFileNames(fileNames); > +#endif Can we make fileNames cross-platform? It seems like a concept that will useful for other platforms and avoiding the #ifdef would make things more readable. > Source/WebKit2/Shared/mac/PasteboardTypes.mm:58 > + NSFilesPromisePboardType, Is this actually new in 10.12? > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3631 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 What about this requires 10.12?
Comment on attachment 295733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295733&action=review > Source/WebCore/platform/DragData.h:111 > + void setFileNames(Vector<String>& fileNames) { m_fileNames = WTFMove(fileNames); } This should be Vector<String>&&, an rvalue reference rather than an lvalue reference.
(In reply to comment #4) > Comment on attachment 295733 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295733&action=review > > > Source/WebCore/ChangeLog:10 > > + Adds the support for handling File Promise type during > > + drag. > > Can you flesh out this ChangeLog (and the others)? Please include > information about why this is 10.12 only. > > > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:1262 > > dragData = DragData(pasteboardName, clientPosition, globalPosition, draggingSourceOperationMask, applicationFlags); > > +#if PLATFORM(MAC) > > + dragData.setFileNames(fileNames); > > +#endif > > Can we make fileNames cross-platform? It seems like a concept that will > useful for other platforms and avoiding the #ifdef would make things more > readable. Ok. > > > Source/WebKit2/Shared/mac/PasteboardTypes.mm:58 > > + NSFilesPromisePboardType, > > Is this actually new in 10.12? FilePromises are not new in 10.12, what is new is the API I'm using to request the fulfillment. I'll explain that in the ChangeLog. > > > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3631 > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 > > What about this requires 10.12?
(In reply to comment #3) > Comment on attachment 295733 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295733&action=review > > > Source/WebCore/platform/mac/DragDataMac.mm:148 > > + return numberOfFiles() == 1; > > Why does having one file mean that it is a promise? Couldn't you have one > file before? I agree this is not clear, I'll change the implementation.
Created attachment 296307 [details] Patch2
Attachment 296307 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3649: No space between ^ and block definition. [whitespace/brackets] [4] ERROR: Source/WebKit/mac/WebView/WebView.mm:6537: No space between ^ and block definition. [whitespace/brackets] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 296307 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=296307&action=review > Source/WebKit2/Shared/mac/PasteboardTypes.mm:58 > + NSFilesPromisePboardType, usually we don't indent like this, just one level. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3642 > + NSLog(@"There was an error with file:%@, skipping %@", fileURL, errorOrNil); I don't love this logging (there should at least be a space after the colon, and maybe it should be reworded -- specifically the word "with" -- the error isn't "with" the file). > Source/WebKit/mac/WebView/WebView.mm:6523 > + __block Vector<String> fileNames; Vector is not thread safe, but you're touching it from an operation queue's arbitrary dispatch thread? Seems like you need some locking, no? Maybe you should have Anders take a peek at this.
(In reply to comment #10) > Comment on attachment 296307 [details] > Patch2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296307&action=review > > > Source/WebKit2/Shared/mac/PasteboardTypes.mm:58 > > + NSFilesPromisePboardType, > > usually we don't indent like this, just one level. > > > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3642 > > + NSLog(@"There was an error with file:%@, skipping %@", fileURL, errorOrNil); > > I don't love this logging (there should at least be a space after the colon, > and maybe it should be reworded -- specifically the word "with" -- the error > isn't "with" the file). > > > Source/WebKit/mac/WebView/WebView.mm:6523 > > + __block Vector<String> fileNames; > > Vector is not thread safe, but you're touching it from an operation queue's > arbitrary dispatch thread? Seems like you need some locking, no? Maybe you > should have Anders take a peek at this. It is one operation thread fulfilling the promise one file at a time, there should be no issue here. I'll CC Anders to take a look if you don't feel comfortable with this.
Comment on attachment 296307 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=296307&action=review > Source/WebKit/mac/WebView/WebView.mm:6525 > + [draggingInfo enumerateDraggingItemsWithOptions:0 forView:self classes:@[[NSFilePromiseReceiver class]] searchOptions:@{ } usingBlock:^(NSDraggingItem * __nonnull draggingItem, NSInteger idx, BOOL * __nonnull stop) { Sam wants lambdas to make lifetimes easier. > Source/WebKit/mac/WebView/WebView.mm:6534 > + fileNames.append(fileURL.path); Move this into the main thread block below.
Created attachment 297146 [details] Patch3 Uses lambdas and addresses non-thread safe use of vector.
Comment on attachment 297146 [details] Patch3 Attachment 297146 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2724669 New failing tests: imported/w3c/web-platform-tests/IndexedDB/interfaces.worker.html
Created attachment 297153 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 297146 [details] Patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=297146&action=review > Source/WebCore/platform/mac/DragDataMac.mm:149 > + platformStrategies()->pasteboardStrategy()->getPathnamesForType(files, String(NSFilesPromisePboardType), m_pasteboardName); How are you getting away without the macOS version #ifdef here but not in the WK2 code? > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3661 > + NSArray *files = [draggingInfo.draggingPasteboard propertyListForType:NSFilenamesPboardType]; You shouldn't just cast/assume like this; check the type! > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3670 > + size_t fileCount = ((NSArray *)[draggingInfo.draggingPasteboard propertyListForType:NSFilesPromisePboardType]).count; You shouldn't just cast/assume like this; check the type! > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3680 > + NSLog(@"There was an error with file:%@, skipping %@", fileURL, errorOrNil); Probably should have a space after the colon > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3683 > + String path(fileURL.path); Is there a reason to do this on this thread? Might as well just capture fileURL and do it there, instead, no? > Source/WebKit/mac/WebView/WebView.mm:6523 > + size_t fileCount = ((NSArray *)[draggingInfo.draggingPasteboard propertyListForType:NSFilesPromisePboardType]).count; You shouldn't just cast like this; check the type!
Comment on attachment 297146 [details] Patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=297146&action=review > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3672 > + NSURL *dropLocation = [NSURL fileURLWithPath:@"/tmp" isDirectory:YES]; I don't think you're allowed to assume that /tmp is the right temporary directory, and should use NSTemporaryDirectory or something
Created attachment 297949 [details] Patch4 Addressing latest comments.
Comment on attachment 297949 [details] Patch4 You talked to Anders, about this, I know -- is there a reason you're using new/delete instead of unique_ptr or some such?
Committed revision 210287
Comment on attachment 297949 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=297949&action=review Some ideas for refinement. > Source/WebCore/platform/DragData.h:108 > + void setFileNames(Vector<String>& fileNames) { m_fileNames = WTFMove(fileNames); } This argument should be a rvalue reference, &&, not an lvalue reference, &. > Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:1260 > + dragData.setFileNames(fileNames); This will need a WTFMove. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3642 > + WebCore::DragData *dragData = new WebCore::DragData(draggingInfo, client, global, static_cast<WebCore::DragOperation>(draggingInfo.draggingSourceOperationMask), applicationFlagsForDrag(m_view, draggingInfo)); The * should be next to the type, DragData*. Or better, just use auto*. But also, this should probably use unique_ptr instead of hand-written delete calls. It’s too easy to have a storage leak otherwise. And we can use WTFMove to transfer ownership into the lambda. In fact, we can probably do that for a DragData too if we make sure we have a move constructor for it, eliminating the need to allocate on the heap at all. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3658 > + Vector<String> fileNames; > + > + for (NSString *file in files) > + fileNames.append(file); Would be better to use reserveInitialCapacity and uncheckedAppend here. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3669 > + Vector<String> *fileNames = new Vector<String>; Spae is in the wrong place, Vector<String>*, or better, auto*. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3672 > + [draggingInfo enumerateDraggingItemsWithOptions:0 forView:m_view classes:@[[NSFilePromiseReceiver class]] searchOptions:@{ } usingBlock:^(NSDraggingItem * __nonnull draggingItem, NSInteger idx, BOOL * __nonnull stop) { The "idx" and "stop" seem unused. Can we leave out the argument names? Should we use a lambda instead of a block so that capturing is explicit? > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3674 > + NSFilePromiseReceiver *item = draggingItem.item; > + NSDictionary *options = @{ }; Seems like we don’t need these local variables. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3676 > + [item receivePromisedFilesAtDestination:dropLocation options:options operationQueue:[NSOperationQueue new] reader:^(NSURL * _Nonnull fileURL, NSError * _Nullable errorOrNil) { Strange that above we use __nonnull and here we use _Nonnull; inconsistent. > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:3682 > + fileNames->append(path.get()); > + if (fileNames->size() == fileCount) { This seems really fragile. I don’t see a solid guarantee that enumerateDraggingItems will return a number of items that exactly matches files.count and if an error occurs, we will be off. If we get too few file names we leak dragData and fileNames. If we get too many file names, we dereference a deleted fileNames pointer. Is there some better way to write this that is less fragile?
Reverted r210287 for reason: This change caused editing test failures on macOS. Committed r210300: <http://trac.webkit.org/changeset/210300>
Committed revision 210360 with fix for failing tests.