Bug 188496 – [Attachment Support] Augment _WKAttachment SPI to handle NSFileWrappers in addition to NSData

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Augment _WKAttachment SPI to handle NSFileWrappers in addition to NSData.

[Radar WebKit Bug Importer]

<rdar://problem/43216836>

[Wenson Hsieh]

Created attachment 347345 [details]
First pass

[Wenson Hsieh]

Created attachment 347346 [details]
First pass (after rebasing)

[Wenson Hsieh]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

Created attachment 347361 [details]
Fix WK1 attachment test and add a WK2 API test

[Wenson Hsieh]

Created attachment 347499 [details]
Patch

[Wenson Hsieh]

Created attachment 347505 [details]
Rebase and try to fix Windows builds

[Wenson Hsieh]

Created attachment 347506 [details]
Try to fix Windows/GTK builds

[Wenson Hsieh]

Created attachment 347517 [details]
Try to fix Windows/GTK builds (2)

[Wenson Hsieh]

Created attachment 347522 [details]
Fix iOS build

[Wenson Hsieh]

Created attachment 347529 [details]
Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT

[Tim Horton]

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

[Wenson Hsieh]

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

👍

[Wenson Hsieh]

Created attachment 347600 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 347600 [details]
Patch for EWS

Clearing flags on attachment: 347600

Committed r235137: <https://trac.webkit.org/changeset/235137>