Bug 183314 – [Mac] Teach WebCore::Pasteboard about file promise drags

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Mac] Teach WebCore::Pasteboard about file promise drags

[Andy Estes]

Created attachment 334963 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/38105493>

[Andy Estes]

Created attachment 334967 [details]
Patch

[Andy Estes]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Andy Estes]

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

[Darin Adler]

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.

[Andy Estes]

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

[Andy Estes]

Created attachment 335035 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 335035 [details]
Patch

Clearing flags on attachment: 335035

Committed r229297: <https://trac.webkit.org/changeset/229297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.