Bug 179013 – [Attachment Support] Implement WKWebView SPI for inserting attachment elements

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Add native SPI for inserting attachment elements.

[Wenson Hsieh]

<rdar://problem/35249668>

[Wenson Hsieh]

Created attachment 325354 [details]
First pass

[Wenson Hsieh]

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:452
> +- (_WKAttachment *)_insertAttachmentWithFilename:(NSString *)filename contentType:(NSString *)contentType data:(NSData *)data options:(_WKAttachmentDisplayOptions *)options completion:(void(^)(BOOL success))completionHandler;

(Just realized I'm missing availability macros here — will add.)

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:451
> +@interface WKWebView (AttachmentSupport)

Why is this a category separate from WKPrivate?

[Wenson Hsieh]

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:451
>> +@interface WKWebView (AttachmentSupport)
> 
> Why is this a category separate from WKPrivate?

It doesn't have to be — the purpose was just to contain all of the attachment-related SPIs under one category. I'll move this into WKPrivate.

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:48
> +@property (nonatomic, copy, readonly) NSString *uniqueIdentifier;

What is the purpose of this property? It’s strange for a mutable, copyable object to have a “unique” identifier like that. Does making a copy preserve the identifier? If so, how is it “unique”?

[Wenson Hsieh]

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:48
>> +@property (nonatomic, copy, readonly) NSString *uniqueIdentifier;
> 
> What is the purpose of this property? It’s strange for a mutable, copyable object to have a “unique” identifier like that. Does making a copy preserve the identifier? If so, how is it “unique”?

Ah, good point — there shouldn't be any need to expose the attachment's unique identifier to clients.

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachmentInternal.h:30
> +@protocol _WKAttachmentDelegate <NSObject>

Conventionally, internal types are not underscore-prefixed.

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachmentInternal.h:36
> +@interface _WKAttachment ()

This is an unusual setup, I think. Normally, Objective-C API objects are wrappers for C++ API objects. What are the advantages doing things this way? What are the advantages of the internal delegate abstraction?

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

> Source/WebCore/PAL/pal/spi/FileSizeFormatter.cpp:39
> +    if (size < 1 << 10)
> +        return String::format("%tu bytes", size);
> +    if (size < 1 << 20)
> +        return String::format("%.1f KB", size / 1024.);
> +    if (size < 1 << 30)
> +        return String::format("%.1f MB", size / (1024. * 1024.));
> +    return String::format("%.1f GB", size / (1024. * 1024. * 1024.));

Shouldn’t these be KiB, MiB and GiB? And shouldn’t these strings be localizable?

[Wenson Hsieh]

Comment on attachment 325354 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=325354&action=review

>> Source/WebCore/PAL/pal/spi/FileSizeFormatter.cpp:39
>> +    return String::format("%.1f GB", size / (1024. * 1024. * 1024.));
> 
> Shouldn’t these be KiB, MiB and GiB? And shouldn’t these strings be localizable?

Fixed the former by actually making this in KB/MB/GB. Filed <https://bugs.webkit.org/show_bug.cgi?id=179019> regarding the latter — there doesn't appear to be a way of adding localizable strings in PAL, since LocalizedStrings currently resides in WebCore. I don't think it makes sense to do this work in the same patch, so let's add a FIXME here referencing the bug. We can additionally stub this method out and just return emptyString() here, if we really don't want unlocalized strings being returned from fileSizeDescription on non-Cocoa platforms.

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachmentInternal.h:30
>> +@protocol _WKAttachmentDelegate <NSObject>
> 
> Conventionally, internal types are not underscore-prefixed.

Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachmentInternal.h:36
>> +@interface _WKAttachment ()
> 
> This is an unusual setup, I think. Normally, Objective-C API objects are wrappers for C++ API objects. What are the advantages doing things this way? What are the advantages of the internal delegate abstraction?

Regarding delegation: originally, I added SPI to manipulate attachments on WKWebView, and had each SPI method take a WKAttachment as an argument. I then refactored this SPI to make it more object-oriented by putting these methods on WKAttachment instead. In doing so, each attachment now needs to call out to its WKWebView, passing itself as an argument. The delegate pattern (where the WKWebView is a delegate for the attachment) seemed to be an appropriate way to implement this, which is why I went with the internal attachment delegate approach. Please let me know if you have a cleaner alternative in mind!

Also, I'm not entirely sure what you mean by "wrapper for C++ API object". WKPreviewElementInfo and _WKActivatedElementInfo, for instance, don't seem to be backed by any C++ API objects (though _WKActivatedElementInfo is effectively a snapshot of the InteractionInformationAtPosition struct, if that's close to what you're referring to by "wrapper for C++ API object"). Are you proposing that we should implement a WK2 C API version of these objects, and then implement the ObjC version of the API on top of it?

[Wenson Hsieh]

Created attachment 325378 [details]
Try to fix builds (and address some review feedback).

[Sam Weinig]

Comment on attachment 325378 [details]
Try to fix builds (and address some review feedback).

View in context: https://bugs.webkit.org/attachment.cgi?id=325378&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:35
> +    _WKAttachmentDisplayModeIcon,
> +    _WKAttachmentDisplayModeImage

To make this work with non-image attachments that have two representations, we could base the naming on the Mac context menu which switches between “View as Icon” and “View in Place”.  That way, this would work for videos (or perhaps things like keynote files in the future).

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:47
> +@interface _WKAttachment : NSObject <NSCopying>

Why is this <NSCopying>?

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:54
> +@implementation _WKAttachment {
> +    RetainPtr<NSString> _uniqueIdentifier;
> +    WeakObjCPtr<id <WKAttachmentDelegate>> _delegate;
> +}

I think this should have a c++ implementation, like all our other types, if only for consistency.

[Wenson Hsieh]

(In reply to Sam Weinig from comment #13)
> Comment on attachment 325378 [details]
> Try to fix builds (and address some review feedback).
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325378&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:35
> > +    _WKAttachmentDisplayModeIcon,
> > +    _WKAttachmentDisplayModeImage
> 
> To make this work with non-image attachments that have two representations,
> we could base the naming on the Mac context menu which switches between
> “View as Icon” and “View in Place”.  That way, this would work for videos
> (or perhaps things like keynote files in the future).

Sounds good! Changed to _WKAttachmentDisplayModeAuto, _WKAttachmentDisplayModeInPlace, and _WKAttachmentDisplayModeAsIcon.

> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:47
> > +@interface _WKAttachment : NSObject <NSCopying>
> 
> Why is this <NSCopying>?

Whoops, I meant to remove this. Removed.

> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.mm:54
> > +@implementation _WKAttachment {
> > +    RetainPtr<NSString> _uniqueIdentifier;
> > +    WeakObjCPtr<id <WKAttachmentDelegate>> _delegate;
> > +}
> 
> I think this should have a c++ implementation, like all our other types, if
> only for consistency.

Yep! I've added an API::Attachment to back _WKAttachment (patch forthcoming).

[Wenson Hsieh]

Created attachment 325390 [details]
Back _WKAttachment with API::Attachment.

[Wenson Hsieh]

Created attachment 325393 [details]
Fix 32-bit Mac build

[Wenson Hsieh]

Created attachment 325400 [details]
Fix 32-bit Mac build (v2)

[Wenson Hsieh]

Created attachment 325405 [details]
Fix 32-bit Mac build (v3)

[Wenson Hsieh]

Created attachment 325410 [details]
Another attempt to fix 32-bit Mac build

[Wenson Hsieh]

Created attachment 325414 [details]
Guard _WKAttachment.mm with WK_API_ENABLED.

[Wenson Hsieh]

Created attachment 325421 [details]
Fix the Windows build

[Tim Horton]

Comment on attachment 325421 [details]
Fix the Windows build

View in context: https://bugs.webkit.org/attachment.cgi?id=325421&action=review

> Source/WebCore/PAL/pal/FileSizeFormatter.cpp:41
> +        return String::format("%.1f MB", size / (1000000.));
> +    return String::format("%.1f GB", size / (1000000000.));

What's up with the extra parens around the denominators?

> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:163
> +    memcpy(sharedMemoryBuffer->data(), buffer->data(), buffer->size());

This seems wrong, like it's defeating the whole point of shm. Let's poke around and/or see what Brady or Alexey or Sam or someone have to say.

> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:38
> +typedef void(^_WKAttachmentDataCompletionHandler)(NSData *, NSError *);

What's this for?

[Tim Horton]

(In reply to Tim Horton from comment #22)
> > Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:163
> > +    memcpy(sharedMemoryBuffer->data(), buffer->data(), buffer->size());
> 
> This seems wrong, like it's defeating the whole point of shm. Let's poke
> around and/or see what Brady or Alexey or Sam or someone have to say.

OH RIGHT! SharedBufferDataReference

[Wenson Hsieh]

Comment on attachment 325421 [details]
Fix the Windows build

View in context: https://bugs.webkit.org/attachment.cgi?id=325421&action=review

>> Source/WebCore/PAL/pal/FileSizeFormatter.cpp:41
>> +    return String::format("%.1f GB", size / (1000000000.));
> 
> What's up with the extra parens around the denominators?

Good catch — removed. (I was previously multiplying 1000. by itself for the larger denominations, but then decided that didn't yield much extra clarity and removed it, but apparently forgot to remove the parens around it).

>> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:163
>> +    memcpy(sharedMemoryBuffer->data(), buffer->data(), buffer->size());
> 
> This seems wrong, like it's defeating the whole point of shm. Let's poke around and/or see what Brady or Alexey or Sam or someone have to say.

Changed to use SharedBufferDataReference here — thanks!

>> Source/WebKit/UIProcess/API/Cocoa/_WKAttachment.h:38
>> +typedef void(^_WKAttachmentDataCompletionHandler)(NSData *, NSError *);
> 
> What's this for?

Whoops, that's for a subsequent patch :) Removed.

[Wenson Hsieh]

Created attachment 325427 [details]
Fix the Windows build (2)

[WebKit Commit Bot]

Comment on attachment 325427 [details]
Fix the Windows build (2)

Clearing flags on attachment: 325427

Committed r224238: <https://trac.webkit.org/changeset/224238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.