RESOLVED FIXED 183314
[Mac] Teach WebCore::Pasteboard about file promise drags
https://bugs.webkit.org/show_bug.cgi?id=183314
Summary [Mac] Teach WebCore::Pasteboard about file promise drags
Andy Estes
Reported 2018-03-03 12:03:51 PST
[Mac] Teach WebCore::Pasteboard about file promise drags
Attachments
Patch (63.19 KB, patch)
2018-03-03 12:59 PST, Andy Estes
no flags
Patch (63.41 KB, patch)
2018-03-03 15:11 PST, Andy Estes
no flags
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
Patch (64.21 KB, patch)
2018-03-05 15:28 PST, Andy Estes
no flags
Andy Estes
Comment 1 2018-03-03 12:59:15 PST
Radar WebKit Bug Importer
Comment 2 2018-03-03 12:59:50 PST
Andy Estes
Comment 3 2018-03-03 15:11:44 PST
Andy Estes
Comment 4 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.
EWS Watchlist
Comment 5 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
EWS Watchlist
Comment 6 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
Andy Estes
Comment 7 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.
Darin Adler
Comment 8 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.
Andy Estes
Comment 9 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!
Andy Estes
Comment 10 2018-03-05 15:28:22 PST
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2018-03-05 17:36:37 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.