[webkit-reviews] review granted: [Bug 175638] [iOS] Respect type fidelities when copying image elements to the pasteboard : [Attachment 318320] Fix iOS 10 build and rebase on master

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 175638: [iOS] Respect type fidelities when copying image elements to the
pasteboard
https://bugs.webkit.org/show_bug.cgi?id=175638

Attachment 318320: Fix iOS 10 build and rebase on master

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




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 318320
  --> https://bugs.webkit.org/attachment.cgi?id=318320
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(linkEleme
nt->attributeWithoutSynchronization(HTMLNames::hrefAttr)));

If you did that, you can just call
downcast<HTMLAnchorElement>(anchorElement)->href(),