Bug 175638 - [iOS] Respect type fidelities when copying image elements to the pasteboard
Summary: [iOS] Respect type fidelities when copying image elements to the pasteboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-16 13:34 PDT by Wenson Hsieh
Modified: 2017-08-20 01:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (47.04 KB, patch)
2017-08-16 15:21 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on master (46.91 KB, patch)
2017-08-16 16:34 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix iOS 10 build (47.20 KB, patch)
2017-08-16 19:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix iOS 10 build and rebase on master (46.86 KB, patch)
2017-08-16 19:42 PDT, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI (51.73 KB, patch)
2017-08-17 09:24 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.07 MB, application/zip)
2017-08-17 10:56 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-08-16 13:34:26 PDT
<rdar://problem/26556043>
Comment 1 Wenson Hsieh 2017-08-16 15:21:33 PDT
Created attachment 318292 [details]
Patch
Comment 2 Wenson Hsieh 2017-08-16 16:34:36 PDT
Created attachment 318304 [details]
Rebase on master
Comment 3 Ryosuke Niwa 2017-08-16 19:06:17 PDT
So iOS bots are still on iOS 10 so we can't quite land this patch yet :(
Comment 4 Wenson Hsieh 2017-08-16 19:29:25 PDT
We just need to def out code that requires the iOS 11 SDK. Our internal bots build iOS 11, but OpenSource is still using iOS 10. I attempted to do it in this patch, but it looks like I missed a spot :/
Comment 5 Wenson Hsieh 2017-08-16 19:35:01 PDT
Created attachment 318319 [details]
Fix iOS 10 build
Comment 6 Wenson Hsieh 2017-08-16 19:42:38 PDT
Created attachment 318320 [details]
Fix iOS 10 build and rebase on master
Comment 7 Ryosuke Niwa 2017-08-16 23:09:09 PDT
Comment on attachment 318320 [details]
Fix iOS 10 build and rebase on master

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:214
> +static RetainPtr<NSDictionary> richTextRepresentationsForPasteboardWebContent(const PasteboardWebContent& content)

Could you define these static functions slightly higher up so that the diff looks saner?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:333
> +    if (!pasteboardImage.url.url.isEmpty()) {

Should we add a FIXME about preferring URL over image if the user was trying to copy the hyperlink instead of image itself?
Or you don't think that's necessary?
If there's a reason you think it's always good to put image before URL, it would be nice to explain that in the change log.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:335
> +        NSURL *nsURL = pasteboardImage.url.url;
> +        if (nsURL)

Nit: declare nsURL inside if (~).

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:369
> +        if (UTTypeConformsTo((__bridge CFStringRef)pasteboardTypeAsNSString, kUTTypeURL) || UTTypeConformsTo((__bridge CFStringRef)pasteboardTypeAsNSString, kUTTypeText))

Can we convert pasteboardTypeAsNSString in a separate line to make this expression easier to read?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2632
> +            if (auto* linkElement = containingLinkElement(&element)) {

I know you're just moving the code around but it's strange that containingLinkElement checks for isLink...
isLink also matches HTMLLinkElement but I don't think that's the intention here.
I think it's better to explicitly check for hasTagQName(HTMLNames::aTag) or is<HTMLAnchorElement>(~) instead.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2633
> +                url = linkElement->document().completeURL(stripLeadingAndTrailingHTMLSpaces(linkElement->attributeWithoutSynchronization(HTMLNames::hrefAttr)));

If you did that, you can just call downcast<HTMLAnchorElement>(anchorElement)->href(),
Comment 8 Wenson Hsieh 2017-08-17 08:08:20 PDT
Comment on attachment 318320 [details]
Fix iOS 10 build and rebase on master

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

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:214
>> +static RetainPtr<NSDictionary> richTextRepresentationsForPasteboardWebContent(const PasteboardWebContent& content)
> 
> Could you define these static functions slightly higher up so that the diff looks saner?

Sure! Fixed.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:333
>> +    if (!pasteboardImage.url.url.isEmpty()) {
> 
> Should we add a FIXME about preferring URL over image if the user was trying to copy the hyperlink instead of image itself?
> Or you don't think that's necessary?
> If there's a reason you think it's always good to put image before URL, it would be nice to explain that in the change log.

Yeah, this should be documented with a FIXME here. Added.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:335
>> +        if (nsURL)
> 
> Nit: declare nsURL inside if (~).

👍

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:369
>> +        if (UTTypeConformsTo((__bridge CFStringRef)pasteboardTypeAsNSString, kUTTypeURL) || UTTypeConformsTo((__bridge CFStringRef)pasteboardTypeAsNSString, kUTTypeText))
> 
> Can we convert pasteboardTypeAsNSString in a separate line to make this expression easier to read?

👍

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2632
>> +            if (auto* linkElement = containingLinkElement(&element)) {
> 
> I know you're just moving the code around but it's strange that containingLinkElement checks for isLink...
> isLink also matches HTMLLinkElement but I don't think that's the intention here.
> I think it's better to explicitly check for hasTagQName(HTMLNames::aTag) or is<HTMLAnchorElement>(~) instead.

Ah, I see. containingLinkElement is only used in a couple of places in this file, both of which walk up the DOM in search of an enclosing anchor around a hit-tested img element. Given your explanation, it looks like we should tweak containingLinkElement to check is<HTMLAnchorElement>.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2633
>> +                url = linkElement->document().completeURL(stripLeadingAndTrailingHTMLSpaces(linkElement->attributeWithoutSynchronization(HTMLNames::hrefAttr)));
> 
> If you did that, you can just call downcast<HTMLAnchorElement>(anchorElement)->href(),

👍 much cleaner!
Comment 9 Wenson Hsieh 2017-08-17 09:24:55 PDT
Created attachment 318373 [details]
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI
Comment 10 Build Bot 2017-08-17 10:56:06 PDT
Comment on attachment 318373 [details]
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI

Attachment 318373 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4331137

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2017-08-17 10:56:08 PDT
Created attachment 318383 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 12 Wenson Hsieh 2017-08-17 11:04:13 PDT
Comment on attachment 318373 [details]
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI

The failures in imported/w3c/web-platform-tests/ look unrelated. Going to see what CQ thinks...
Comment 13 WebKit Commit Bot 2017-08-17 11:34:19 PDT
Comment on attachment 318373 [details]
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI

Clearing flags on attachment: 318373

Committed r220865: <http://trac.webkit.org/changeset/220865>