Summary: | [Attachment Support] Augment _WKAttachment SPI to handle NSFileWrappers in addition to NSData | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | aestes, bdakin, commit-queue, ews-watchlist, mitz, rniwa, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | 188497 | ||||||||||||||||||||||||||||
Bug Blocks: | 188715 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2018-08-12 22:13:24 PDT
Created attachment 347345 [details]
First pass
Created attachment 347346 [details]
First pass (after rebasing)
> Regressions: Unexpected text-only failures (1)
> editing/pasteboard/drag-files-to-editable-element-as-attachment.html [ Failure ]
>
> Regressions: Unexpected timeouts (1)
> editing/pasteboard/drag-file-promises-to-editable-element-as-attachment.html [ Timeout ]
These attachment element tests are failing because I removed logic for creating File objects for inserted attachment elements when dropping file paths. A couple of thoughts:
(1) We could preserve WebKit1 behavior by adding a hook on WebEditorClient that's something to the effect of, "supports client-side attachment data", and we'd return false in WebKitLegacy; then, when we go and set the file, we could either make a new File and set it if this flag returns false, otherwise we'd propagate attachment data to the client.
(2) AFAIK, no client actually depends on us creating and setting HTMLAttachmentElement's file when dropping file paths. We could just refactor the test to not inspect the attachment's file. I could also reimplement these tests as API tests using DragAndDropSimulator in WebKit2.
Comment on attachment 347346 [details] First pass (after rebasing) Attachment 347346 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8888606 New failing tests: editing/pasteboard/drag-file-promises-to-editable-element-as-attachment.html editing/pasteboard/drag-files-to-editable-element-as-attachment.html Created attachment 347350 [details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 347346 [details] First pass (after rebasing) Attachment 347346 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8889224 New failing tests: editing/pasteboard/drag-file-promises-to-editable-element-as-attachment.html editing/pasteboard/drag-files-to-editable-element-as-attachment.html Created attachment 347353 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 347361 [details]
Fix WK1 attachment test and add a WK2 API test
Created attachment 347499 [details]
Patch
Created attachment 347505 [details]
Rebase and try to fix Windows builds
Created attachment 347506 [details]
Try to fix Windows/GTK builds
Created attachment 347517 [details]
Try to fix Windows/GTK builds (2)
Created attachment 347522 [details]
Fix iOS build
Created attachment 347529 [details]
Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT
Comment on attachment 347529 [details] Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT View in context: https://bugs.webkit.org/attachment.cgi?id=347529&action=review > Source/WebCore/ChangeLog:88 > + (): Deleted. Oh really > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:686 > + FileSystem::getFileSize(path, fileSize); > + auto contentType = File::contentTypeForFile(path); How does this all go if we don't have access to the file? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4428 > + auto typeIdentifier = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (CFStringRef)pathExtension, NULL)); > + contentType = (NSString *)CFAutorelease(UTTypeCopyPreferredTagWithClass(typeIdentifier.get(), kUTTagClassMIMEType)); Maybe use UTIUtilities for less typing? > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:73 > + if (![_fileWrapper isRegularFile]) > + return nil; What other kinds of files are there? How sure are we that we don't want them? > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:99 > + auto typeIdentifier = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (CFStringRef)extension, nil)); Moar UTIUtilities > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:169 > + auto fileSize = [[[fileWrapper fileAttributes] objectForKey:NSFileSize] unsignedLongLongValue]; Subscripts instead of objectForKey? > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:174 > + _attachment->updateAttributes(fileSize, contentType, [fileWrapper preferredFilename], [capturedBlock = makeBlockPtr(completionHandler)] (auto error) { fileWrapper.preferredFilename? (dots!) Comment on attachment 347529 [details] Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT View in context: https://bugs.webkit.org/attachment.cgi?id=347529&action=review >> Source/WebCore/ChangeLog:88 >> + (): Deleted. > > Oh really 😐 (Removed) >> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:686 >> + auto contentType = File::contentTypeForFile(path); > > How does this all go if we don't have access to the file? We'll be unable to infer the content type for the file path if we don't have a sandbox extension :/ That being said, this code already assumes we have a sandbox extension for the files we're reading (we previously created File objects from the path). >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4428 >> + contentType = (NSString *)CFAutorelease(UTTypeCopyPreferredTagWithClass(typeIdentifier.get(), kUTTagClassMIMEType)); > > Maybe use UTIUtilities for less typing? Yes! Good call (I think MIMETypeRegistry::getMIMETypeForExtension is what I want here) >> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:73 >> + return nil; > > What other kinds of files are there? How sure are we that we don't want them? There are symlinks and directories. We'll eventually want to support at least directories, and I'm going to make this work in a followup. For now, the goal of this patch is to introduce the new interfaces and refactor our existing attachment editing infrastructure without disturbing the behavior of the existing SPI used by Mail, which currently only takes an NSData when inserting attachments (equivalent to creating an NSFileWrapper with regular file contents). >> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:99 >> + auto typeIdentifier = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassFilenameExtension, (CFStringRef)extension, nil)); > > Moar UTIUtilities 👍 >> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:169 >> + auto fileSize = [[[fileWrapper fileAttributes] objectForKey:NSFileSize] unsignedLongLongValue]; > > Subscripts instead of objectForKey? Sounds good. >> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:174 >> + _attachment->updateAttributes(fileSize, contentType, [fileWrapper preferredFilename], [capturedBlock = makeBlockPtr(completionHandler)] (auto error) { > > fileWrapper.preferredFilename? (dots!) 👍 Created attachment 347600 [details]
Patch for EWS
Comment on attachment 347600 [details] Patch for EWS Clearing flags on attachment: 347600 Committed r235137: <https://trac.webkit.org/changeset/235137> |