Bug 171154 - Support writing link titles to the pasteboard when beginning data interaction on a link
Summary: Support writing link titles to the pasteboard when beginning data interaction...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-21 18:19 PDT by Wenson Hsieh
Modified: 2017-04-22 05:35 PDT (History)
5 users (show)

See Also:


Attachments
Part (1/3) (24.29 KB, patch)
2017-04-21 18:48 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Attempt to fix 32-bit iOS. (23.77 KB, patch)
2017-04-21 20:16 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Really fix iOS build (23.81 KB, patch)
2017-04-21 20:41 PDT, Wenson Hsieh
aestes: review+
Details | Formatted Diff | Diff
Patch for landing (24.15 KB, patch)
2017-04-21 22:37 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 2017-04-21 18:19:48 PDT
Work towards <rdar://problem/31356937>
Comment 1 Wenson Hsieh 2017-04-21 18:48:08 PDT
Created attachment 307860 [details]
Part (1/3)
Comment 2 Wenson Hsieh 2017-04-21 20:16:09 PDT
Created attachment 307869 [details]
Attempt to fix 32-bit iOS.
Comment 3 Wenson Hsieh 2017-04-21 20:41:01 PDT
Created attachment 307874 [details]
Really fix iOS build
Comment 4 Andy Estes 2017-04-21 21:43:29 PDT
Comment on attachment 307874 [details]
Really fix iOS build

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:42
> +#if ENABLE(DATA_INTERACTION)
> +#import <UIKit/NSURL+UIItemProvider.h>
> +#endif

This is fine for now, but the build will break once we start building against the Public iOS 11 SDK because NSURL+UIItemProvider.h is a Private header. We'll need to make a SPI header for this.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:296
> +    RetainPtr<NSMutableArray> objectRepresentations = adoptNS([[NSMutableArray alloc] init]);

This is one of the cases where I prefer auto, since the type is clear from the right hand side.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:299
> +    NSURL *nsURL = (NSURL *)url.url;
> +    if (nsURL) {

These two lines could be:

    if (NSURL *nsURL = (NSURL *)url.url) {

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:307
> +    [m_pasteboard setItemsFromObjectRepresentations:@[[WebPasteboardItemData itemWithRepresentingObjects:objectRepresentations.get() additionalData:@{ }]]];

Can you use nil for the additionalData argument, or does it really need to be an empty NSDictionary?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:176
> +            [itemProvider registerDataRepresentationForTypeIdentifier:typeIdentifier visibility:UIItemProviderRepresentationOptionsVisibilityAll loadHandler:^NSProgress *(void (^completionHandler)(NSData *, NSError *))

Can you use the UIItemProviderDataLoadHandler typedef here?
Comment 5 Wenson Hsieh 2017-04-21 22:28:55 PDT
Comment on attachment 307874 [details]
Really fix iOS build

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

Thanks for taking a look!

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:42
>> +#endif
> 
> This is fine for now, but the build will break once we start building against the Public iOS 11 SDK because NSURL+UIItemProvider.h is a Private header. We'll need to make a SPI header for this.

Good point. I think the intention is to make NSURL+UIItemProvider.h public at some point (I suppose that begs the question, where will _title be defined :|). But if PoR is for this to remain private, I'll roll this into UIKitSPI.h, defining _title in a private category on NSURL for non-internal SDKs.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:296
>> +    RetainPtr<NSMutableArray> objectRepresentations = adoptNS([[NSMutableArray alloc] init]);
> 
> This is one of the cases where I prefer auto, since the type is clear from the right hand side.

Yep! Changed to use auto.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:299
>> +    if (nsURL) {
> 
> These two lines could be:
> 
>     if (NSURL *nsURL = (NSURL *)url.url) {

True! (I can also drop the (NSURL *) as well, since the conversion is implicit). Fixed.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:307
>> +    [m_pasteboard setItemsFromObjectRepresentations:@[[WebPasteboardItemData itemWithRepresentingObjects:objectRepresentations.get() additionalData:@{ }]]];
> 
> Can you use nil for the additionalData argument, or does it really need to be an empty NSDictionary?

Good point. I could use nil here, but I'm actually going to refactor this in the next patch (not up yet :p) so that we don't have to use initialize WebPasteboardItemDatas in this way at all. I'll change it to nil here though.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:176
>> +            [itemProvider registerDataRepresentationForTypeIdentifier:typeIdentifier visibility:UIItemProviderRepresentationOptionsVisibilityAll loadHandler:^NSProgress *(void (^completionHandler)(NSData *, NSError *))
> 
> Can you use the UIItemProviderDataLoadHandler typedef here?

I wanted to avoid using that, since it's marked as deprecated in the UIKit headers (UIItemProvider_Private.h). I agree it looks much cleaner using a typedef though -- I'll declare a typedef at the top of this file and use that here instead.
Comment 6 Wenson Hsieh 2017-04-21 22:37:37 PDT
Created attachment 307887 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-04-21 23:18:56 PDT
Comment on attachment 307887 [details]
Patch for landing

Clearing flags on attachment: 307887

Committed r215663: <http://trac.webkit.org/changeset/215663>