WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(7.53 KB, patch)
2018-06-29 16:44 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(7.56 KB, patch)
2018-06-29 17:48 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch for landing
(7.99 KB, patch)
2018-07-02 14:13 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2018-06-29 14:49:40 PDT
Created
attachment 343947
[details]
Patch
Megan Gardner
Comment 2
2018-06-29 14:50:23 PDT
<
rdar://problem/41451148
>
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
Created
attachment 343969
[details]
Patch
Megan Gardner
Comment 5
2018-06-29 17:48:07 PDT
Created
attachment 343986
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug