WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Andy Estes
Comment 1
2018-03-03 12:59:15 PST
Created
attachment 334963
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2018-03-03 12:59:50 PST
<
rdar://problem/38105493
>
Andy Estes
Comment 3
2018-03-03 15:11:44 PST
Created
attachment 334967
[details]
Patch
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
Created
attachment 335035
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug