RESOLVED FIXED 187194
Enable copy paste on iOS apps for Mac
https://bugs.webkit.org/show_bug.cgi?id=187194
Summary Enable copy paste on iOS apps for Mac
Megan Gardner
Reported 2018-06-29 14:39:23 PDT
Enable copy paste on iOS apps for Mac
Attachments
Patch (7.30 KB, patch)
2018-06-29 14:49 PDT, Megan Gardner
no flags
Patch (7.53 KB, patch)
2018-06-29 16:44 PDT, Megan Gardner
no flags
Patch (7.56 KB, patch)
2018-06-29 17:48 PDT, Megan Gardner
no flags
Patch for landing (7.99 KB, patch)
2018-07-02 14:13 PDT, Megan Gardner
no flags
Megan Gardner
Comment 1 2018-06-29 14:49:40 PDT
Megan Gardner
Comment 2 2018-06-29 14:50:23 PDT
Wenson Hsieh
Comment 3 2018-06-29 15:30:49 PDT
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
Comment 4 2018-06-29 16:44:50 PDT
Megan Gardner
Comment 5 2018-06-29 17:48:07 PDT
Darin Adler
Comment 6 2018-06-29 18:01:59 PDT
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
Comment 7 2018-06-29 18:05:44 PDT
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 8 2018-07-02 13:40:56 PDT
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
Comment 9 2018-07-02 14:13:30 PDT
Created attachment 344134 [details] Patch for landing
WebKit Commit Bot
Comment 10 2018-07-02 14:52:24 PDT
Comment on attachment 344134 [details] Patch for landing Clearing flags on attachment: 344134 Committed r233436: <https://trac.webkit.org/changeset/233436>
WebKit Commit Bot
Comment 11 2018-07-02 14:52:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.