[webkit-reviews] review granted: [Bug 196435] [iOS] Refactor some logic for inserting pasted or dropped virtual card files as attachment elements : [Attachment 366379] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 196435: [iOS] Refactor some logic for inserting pasted or dropped virtual
card files as attachment elements
https://bugs.webkit.org/show_bug.cgi?id=196435

Attachment 366379: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 366379
  --> https://bugs.webkit.org/attachment.cgi?id=366379
Patch

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

> Source/WebCore/platform/ios/PasteboardIOS.mm:204
> +	   // we would prefer to insert an Apple maps link instead.

"Apple Maps link", not "Apple maps link"

> Source/WebCore/platform/ios/PasteboardIOS.mm:256
> +inline static void
readURLAlongsideAttachmentIfNecessary(PasteboardWebContentReader& reader,
PasteboardStrategy& strategy, const String& typeIdentifier, const String&
pasteboardName, int itemIndex)

I don’t think we need to write inline here. Doesn’t affect the compiler’s
decision on whether to inline in practice these days, just the ability to
include the same function in multiple translation units.

> Source/WebCore/platform/ios/PasteboardIOS.mm:260
> +    auto cfType = typeIdentifier.createCFString();
> +    if (!UTTypeConformsTo(cfType.get(), kUTTypeVCard))
> +	   return;

I think this reads better without the local variable. Without RetainPtr, we
would need it so we can write the release, but with RetainPtr it can just be
one expression.

> Source/WebCore/platform/ios/PasteboardIOS.mm:278
> +    auto cfType = info.contentTypeForHighestFidelityItem().createCFString();
> +    if (UTTypeConformsTo(cfType.get(), kUTTypeVCard))
> +	   return true;

Ditto. Or if we do have a local variable then I suggest it contain the type as
a WTF::String, use it above for the isEmpty() check, and put the createCFString
call into the call to UTTypeConformsTo.

Also might just return UTTypeConformsTo instead of if (x) return true; return
false;