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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 29 18:01:59 PDT 2018

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

Attachment 343986: Patch


--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 343986
  --> https://bugs.webkit.org/attachment.cgi?id=343986

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
>	   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

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


> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:641
>      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

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

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
> +    [itemProvider
>      [itemProvider
>      [itemProvider setTeamData:self.teamData];
> +#endif

Why? Comment please.

More information about the webkit-reviews mailing list