Bug 183314 - [Mac] Teach WebCore::Pasteboard about file promise drags
Summary: [Mac] Teach WebCore::Pasteboard about file promise drags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-03 12:03 PST by Andy Estes
Modified: 2018-03-05 17:36 PST (History)
12 users (show)

See Also:


Attachments
Patch (63.19 KB, patch)
2018-03-03 12:59 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (63.41 KB, patch)
2018-03-03 15:11 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.13 MB, application/zip)
2018-03-03 19:04 PST, EWS Watchlist
no flags Details
Patch (64.21 KB, patch)
2018-03-05 15:28 PST, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2018-03-03 12:03:51 PST
[Mac] Teach WebCore::Pasteboard about file promise drags
Comment 1 Andy Estes 2018-03-03 12:59:15 PST
Created attachment 334963 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-03-03 12:59:50 PST
<rdar://problem/38105493>
Comment 3 Andy Estes 2018-03-03 15:11:44 PST
Created attachment 334967 [details]
Patch
Comment 4 Andy Estes 2018-03-03 15:29:37 PST
Comment on attachment 334967 [details]
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:88
> +- (void)dealloc

I forgot to release _draggingSource in here. Will fix before landing.
Comment 5 EWS Watchlist 2018-03-03 19:04:05 PST
Comment on attachment 334967 [details]
Patch

Attachment 334967 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6780229

New failing tests:
editing/pasteboard/drag-file-promises-to-editable-element-as-attachment.html
editing/pasteboard/data-transfer-items-drop-file-promise.html
editing/pasteboard/drag-file-promises-to-editable-element-as-URLs.html
editing/pasteboard/data-transfer-items-drag-drop-file-promise.html
editing/pasteboard/file-input-files-access-promise.html
editing/pasteboard/datatransfer-types-dropping-text-file-promise.html
Comment 6 EWS Watchlist 2018-03-03 19:04:16 PST
Created attachment 334972 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 7 Andy Estes 2018-03-03 21:42:11 PST
(In reply to Build Bot from comment #6)
> Created attachment 334972 [details]
> Archive of layout-test-results from ews202 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews202  Port: win-future  Platform:
> CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Huh, I guess Windows inherits test expectations from mac-wk1. I’ll properly skip these tests on Windows before landing.
Comment 8 Darin Adler 2018-03-04 13:39:16 PST
Comment on attachment 334967 [details]
Patch

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

Great work!

Those FIXMEs are vexing; it would be great to add more test cases demonstrating the FIXME code is doing the wrong thing.

> Source/WebCore/platform/FileSystem.h:159
> -bool hardLinkOrCopyFile(const String& source, const String& destination);
> +WEBCORE_EXPORT bool hardLinkOrCopyFile(const String& source, const String& destination);

Stray thought on seeing this: I wonder if the clients of this function would ever want to clone files on APFS rather than hard linking or copying.

> Source/WebCore/platform/mac/PasteboardMac.mm:279
> +static String toString(const Vector<String>& pathnames)

I don’t think this is a specific enough function name. Maybe instead of thinking of it as a Pasteboard function we should think of it as a generally useful string vector function and call it joinSeparatedByNewlines. Otherwise I think we should call it joinPathnames or something like that.

We can make this function run faster by computing the sum of all the string lengths and then:

    builder.reserveCapacity(totalLength + size - 1);

We need to write the code with overflow-checking arithmetic, though. No need to do that version now, but if we want a more optimal join function we can either do that, or maybe a version that uses StringImpl::createUninitialized.

> Source/WebCore/platform/mac/PasteboardMac.mm:287
> +    StringBuilder builder;
> +    for (size_t i = 0; i < pathnames.size(); ++i) {
> +        if (i)
> +            builder.append('\n');
> +        builder.append(pathnames[i]);
> +    }
> +    return builder.toString();

Could use modern for loop and:

    if (!builder.isEmpty())
        builder.append('\n');

> Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:49
> +@interface DumpRenderTreeFilePromiseReceiver : NSFilePromiseReceiver {

A little sad seeing all this new stuff for DumpRenderTree. Seems a little bit backward-looking. What’s the equivalent going to be for WebKitTestRunner?

> Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:111
> +    while (!FileSystem::hardLinkOrCopyFile(sourceURL.path, destinationURL.path)) {

Not sure we really need to use FileSystem here. This is Cocoa-specific so we could use NSFileManager methods.
Comment 9 Andy Estes 2018-03-04 18:02:28 PST
(In reply to Darin Adler from comment #8)
> > Source/WebCore/platform/mac/PasteboardMac.mm:279
> > +static String toString(const Vector<String>& pathnames)
> 
> I don’t think this is a specific enough function name. Maybe instead of
> thinking of it as a Pasteboard function we should think of it as a generally
> useful string vector function and call it joinSeparatedByNewlines. Otherwise
> I think we should call it joinPathnames or something like that.
> 
> We can make this function run faster by computing the sum of all the string
> lengths and then:
> 
>     builder.reserveCapacity(totalLength + size - 1);
> 
> We need to write the code with overflow-checking arithmetic, though. No need
> to do that version now, but if we want a more optimal join function we can
> either do that, or maybe a version that uses StringImpl::createUninitialized.

I renamed the function for now, but added a comment about how we could improve it and make it more general.

> 
> > Source/WebCore/platform/mac/PasteboardMac.mm:287
> > +    StringBuilder builder;
> > +    for (size_t i = 0; i < pathnames.size(); ++i) {
> > +        if (i)
> > +            builder.append('\n');
> > +        builder.append(pathnames[i]);
> > +    }
> > +    return builder.toString();
> 
> Could use modern for loop and:
> 
>     if (!builder.isEmpty())
>         builder.append('\n');

Done.

> 
> > Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:49
> > +@interface DumpRenderTreeFilePromiseReceiver : NSFilePromiseReceiver {
> 
> A little sad seeing all this new stuff for DumpRenderTree. Seems a little
> bit backward-looking. What’s the equivalent going to be for WebKitTestRunner?

I'm a little sad about this too. My choice here was between this and inventing a Mac drag and drop mechanism for WebKitTestRunner, so I took what seemed like the shortest path to test coverage.

I believe adding drag and drop support to WebKitTestRunner is tracked by these bugs:

https://bugs.webkit.org/show_bug.cgi?id=64285
https://bugs.webkit.org/show_bug.cgi?id=68552

Although these days we'd probably want to do this in terms of UIScriptController rather than EventSender.

> 
> > Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:111
> > +    while (!FileSystem::hardLinkOrCopyFile(sourceURL.path, destinationURL.path)) {
> 
> Not sure we really need to use FileSystem here. This is Cocoa-specific so we
> could use NSFileManager methods.

Changed to use -copyItemAtURL:...

Thanks for the review!
Comment 10 Andy Estes 2018-03-05 15:28:22 PST
Created attachment 335035 [details]
Patch
Comment 11 WebKit Commit Bot 2018-03-05 17:36:36 PST
Comment on attachment 335035 [details]
Patch

Clearing flags on attachment: 335035

Committed r229297: <https://trac.webkit.org/changeset/229297>
Comment 12 WebKit Commit Bot 2018-03-05 17:36:37 PST
All reviewed patches have been landed.  Closing bug.