Bug 188496 - [Attachment Support] Augment _WKAttachment SPI to handle NSFileWrappers in addition to NSData
Summary: [Attachment Support] Augment _WKAttachment SPI to handle NSFileWrappers in ad...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 188497
Blocks: 188715
  Show dependency treegraph
 
Reported: 2018-08-12 22:13 PDT by Wenson Hsieh
Modified: 2018-08-21 17:01 PDT (History)
10 users (show)

See Also:


Attachments
First pass (143.15 KB, patch)
2018-08-16 20:38 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
First pass (after rebasing) (143.17 KB, patch)
2018-08-16 20:47 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (3.02 MB, application/zip)
2018-08-16 22:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (2.32 MB, application/zip)
2018-08-17 00:01 PDT, Build Bot
no flags Details
Fix WK1 attachment test and add a WK2 API test (148.88 KB, patch)
2018-08-17 09:14 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (145.02 KB, patch)
2018-08-20 08:50 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase and try to fix Windows builds (146.43 KB, patch)
2018-08-20 10:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix Windows/GTK builds (146.49 KB, patch)
2018-08-20 10:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix Windows/GTK builds (2) (147.05 KB, patch)
2018-08-20 11:29 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix iOS build (147.33 KB, patch)
2018-08-20 12:02 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT (148.31 KB, patch)
2018-08-20 12:32 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for EWS (149.94 KB, patch)
2018-08-20 19:43 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-08-12 22:13:24 PDT
Augment _WKAttachment SPI to handle NSFileWrappers in addition to NSData.
Comment 1 Radar WebKit Bug Importer 2018-08-12 22:13:45 PDT
<rdar://problem/43216836>
Comment 2 Wenson Hsieh 2018-08-16 20:38:48 PDT
Created attachment 347345 [details]
First pass
Comment 3 Wenson Hsieh 2018-08-16 20:47:36 PDT
Created attachment 347346 [details]
First pass (after rebasing)
Comment 4 Wenson Hsieh 2018-08-16 21:44:51 PDT
> 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 5 Build Bot 2018-08-16 22:33:15 PDT
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
Comment 6 Build Bot 2018-08-16 22:33:17 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2018-08-17 00:01:25 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2018-08-17 00:01:27 PDT Comment hidden (obsolete)
Comment 9 Wenson Hsieh 2018-08-17 09:14:24 PDT
Created attachment 347361 [details]
Fix WK1 attachment test and add a WK2 API test
Comment 10 Wenson Hsieh 2018-08-20 08:50:07 PDT
Created attachment 347499 [details]
Patch
Comment 11 Wenson Hsieh 2018-08-20 10:22:32 PDT
Created attachment 347505 [details]
Rebase and try to fix Windows builds
Comment 12 Wenson Hsieh 2018-08-20 10:35:18 PDT
Created attachment 347506 [details]
Try to fix Windows/GTK builds
Comment 13 Wenson Hsieh 2018-08-20 11:29:56 PDT
Created attachment 347517 [details]
Try to fix Windows/GTK builds (2)
Comment 14 Wenson Hsieh 2018-08-20 12:02:31 PDT
Created attachment 347522 [details]
Fix iOS build
Comment 15 Wenson Hsieh 2018-08-20 12:32:02 PDT
Created attachment 347529 [details]
Guard API::Attachment with ENABLE_ATTACHMENT_ELEMENT
Comment 16 Tim Horton 2018-08-20 15:52:52 PDT
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 17 Wenson Hsieh 2018-08-20 16:03:08 PDT
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!)

👍
Comment 18 Wenson Hsieh 2018-08-20 19:43:41 PDT
Created attachment 347600 [details]
Patch for EWS
Comment 19 WebKit Commit Bot 2018-08-21 14:10:41 PDT
Comment on attachment 347600 [details]
Patch for EWS

Clearing flags on attachment: 347600

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