[webkit-reviews] review granted: [Bug 187194] Enable copy paste on iOS apps for Mac : [Attachment 343986] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 187194: Enable copy paste on iOS apps for Mac
https://bugs.webkit.org/show_bug.cgi?id=187194

Attachment 343986: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 343986
  --> https://bugs.webkit.org/attachment.cgi?id=343986
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.preferredPresen
tationStyle)];
>      [itemProvider setTeamData:self.teamData];
> +#endif

Why? Comment please.