Bug 165204 – Support File Promise during drag for macOS

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 295733 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Sam Weinig]

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?

[Darin Adler]

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.

[Enrica Casucci]

(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?

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 296307 [details]
Patch2

[WebKit Commit Bot]

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.

[Tim Horton]

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.

[Enrica Casucci]

(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.

[Tim Horton]

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.

[Enrica Casucci]

Created attachment 297146 [details]
Patch3

Uses lambdas and addresses non-thread safe use of vector.

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

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!

[Tim Horton]

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

[Enrica Casucci]

Created attachment 297949 [details]
Patch4

Addressing latest comments.

[Tim Horton]

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?

[Enrica Casucci]

Committed revision 210287

[Darin Adler]

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?

[Ryan Haddad]

Reverted r210287 for reason:

This change caused editing test failures on macOS.

Committed r210300: <http://trac.webkit.org/changeset/210300>

[Enrica Casucci]

Committed revision 210360 with fix for failing tests.