Bug 175638 – [iOS] Respect type fidelities when copying image elements to the pasteboard

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/26556043>

[Wenson Hsieh]

Created attachment 318292 [details]
Patch

[Wenson Hsieh]

Created attachment 318304 [details]
Rebase on master

[Ryosuke Niwa]

So iOS bots are still on iOS 10 so we can't quite land this patch yet :(

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 318319 [details]
Fix iOS 10 build

[Wenson Hsieh]

Created attachment 318320 [details]
Fix iOS 10 build and rebase on master

[Ryosuke Niwa]

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(),

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 318373 [details]
Review feedback + estimatedDisplayedSize => preferredPresentationSize in Tools/TestWebKitAPI

[Build Bot]

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.

[Build Bot]

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

[Wenson Hsieh]

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

[WebKit Commit Bot]

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>