Bug 187194 – Enable copy paste on iOS apps for Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Enable copy paste on iOS apps for Mac

[Megan Gardner]

Created attachment 343947 [details]
Patch

[Megan Gardner]

<rdar://problem/41451148>

[Wenson Hsieh]

Comment on attachment 343947 [details]
Patch

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

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:30
> +#if ENABLE(MINIMAL_SIMULATOR)

Since this is a private header, this should be TARGET_OS_IOSMAC instead.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:29
> +#if ENABLE(DATA_INTERACTION) || ENABLE(MINIMAL_SIMULATOR)

Nit - let's update the comment after the #endif as well!

[Megan Gardner]

Created attachment 343969 [details]
Patch

[Megan Gardner]

Created attachment 343986 [details]
Patch

[Darin Adler]

Comment on attachment 343986 [details]
Patch

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:262
> +    [pasteboard setItems:@[itemDictionary.get()]];

Code below uses a style with spaces between the @[ and the ], and it would be nice to be consistent.

I’m also surprised that we want an array with a single item that is a dictionary here, but I’m willing to believe it!

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:366
> +#if !ENABLE(MINIMAL_SIMULATOR)
>          nsURL._title = pasteboardURL.title.isEmpty() ? userVisibleString(pasteboardURL.url) : (NSString *)pasteboardURL.title;
> +#endif

I think we need a comment about why we don’t do that on iOS running on Mac. It seems random and arbitrary without a comment, although arguably more terse and elegant. If the #if was something more along the rough lines of NSURL_SUPPORTS_TITLE, then it would be self explanatory; the comment would go where that is defined instead. I think that would be best. Following a pattern like PASTEBOARD_SUPPORTS_ITEM_PROVIDERS.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:399
> +#if !ENABLE(MINIMAL_SIMULATOR)
>          if (!url.title.isEmpty())
>              nsURL._title = url.title;
> +#endif

Ditto.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:641
> +#if PASTEBOARD_SUPPORTS_ITEM_PROVIDERS && !ENABLE(MINIMAL_SIMULATOR)
>      title = [url _title];

Can we really do without the title? I understand that we can’t use the "_title" property. I am concerned because we may need the title for things like good quality rich links.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:35
> +#define UIItemProvider NSItemProvider
> +#define UIItemProviderReading NSItemProviderReading
> +#define UIItemProviderWriting NSItemProviderWriting
> +#define UIItemProviderRepresentationOptionsVisibilityAll NSItemProviderRepresentationVisibilityAll

Is this the kind of hack we want to do to share code across the platforms? I don’t see this sort of thing elsewhere, but maybe we decided on this the past?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:29
> -#if ENABLE(DATA_INTERACTION)
> +#if ENABLE(DATA_INTERACTION) || ENABLE(MINIMAL_SIMULATOR)

This combination of feature conditional plus platform conditional seems particularly inelegant. Is there a way to do better?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:289
> +#if !ENABLE(MINIMAL_SIMULATOR)
> +    [itemProvider setPreferredPresentationSize:self.preferredPresentationSize];
>      [itemProvider setPreferredPresentationStyle:uiPreferredPresentationStyle(self.preferredPresentationStyle)];
>      [itemProvider setTeamData:self.teamData];
> +#endif

Why? Comment please.

[Darin Adler]

You may be feeling that "a few things have to be off just for now, so I’ll just turn them off knowing I’m coming back here really soon”, but my past experience is that in practice there is lots of code like this many, many years later, so the comments may well be quite valuable. And if not, you can delete them.

[Megan Gardner]

Comment on attachment 343986 [details]
Patch

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

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:36
> +

This is temporary until we switch to NSItemProviders everywhere, which will hopefully be soon.

[Megan Gardner]

Created attachment 344134 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344134 [details]
Patch for landing

Clearing flags on attachment: 344134

Committed r233436: <https://trac.webkit.org/changeset/233436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.