Bug 155074

Summary: Support preview on attachment elements
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebKit2Assignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: sam, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch2 sam: review+

Description Enrica Casucci 2016-03-05 16:23:33 PST 
We want to be able to preview attachment elements, like we do for links and images.

rdar://problem/24806079
Comment 1 Enrica Casucci 2016-03-05 16:28:35 PST 
Created attachment 273108 [details]
Patch
Comment 2 Enrica Casucci 2016-03-05 18:53:38 PST 
Created attachment 273117 [details]
Patch2
Comment 3 Sam Weinig 2016-03-05 19:01:41 PST 
Comment on attachment 273117 [details]
Patch2

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:76
> +- (NSUInteger)_webView:(WKWebView *)webView attachmentIndexElement:(_WKActivatedElementInfo *)element WK_AVAILABLE(NA, WK_IOS_TBA);

This name is a bit awkward.  I think something like, _webView:indexIntoAttachmentListForElement: or something like that.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3712
> +        enum { UIPreviewItemTypeAttachment = 5 };

Even though I suggested this, it now makes me nervous.  I think you should just use the 5 below with a comment, or pick a name that won't conflict with UIKit.
Comment 4 Enrica Casucci 2016-03-06 19:30:01 PST 
(In reply to comment #3)
> Comment on attachment 273117 [details]
> Patch2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273117&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:76
> > +- (NSUInteger)_webView:(WKWebView *)webView attachmentIndexElement:(_WKActivatedElementInfo *)element WK_AVAILABLE(NA, WK_IOS_TBA);
> 
> This name is a bit awkward.  I think something like,
> _webView:indexIntoAttachmentListForElement: or something like that.
> 
I'll change it.
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3712
> > +        enum { UIPreviewItemTypeAttachment = 5 };
> 
> Even though I suggested this, it now makes me nervous.  I think you should
> just use the 5 below with a comment, or pick a name that won't conflict with
> UIKit.

I agree with you, I'll use a different name for the enum.
Comment 5 Enrica Casucci 2016-03-06 19:37:46 PST 
Committed revision 197656.
Comment 6 mitz 2016-03-06 19:44:47 PST 
Comment on attachment 273117 [details]
Patch2

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:75
> +- (NSArray *)_attachmentListForWebView:(WKWebView *)webView WK_AVAILABLE(NA, WK_IOS_TBA);

Can this be WK_ARRAY<> of a more specific type?