Bug 196435 – [iOS] Refactor some logic for inserting pasted or dropped virtual card files as attachment elements

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

Work towards <rdar://problem/48573098>

[Wenson Hsieh]

Created attachment 366379 [details]
Patch

[Darin Adler]

Comment on attachment 366379 [details]
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;

[Wenson Hsieh]

Comment on attachment 366379 [details]
Patch

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

Thanks for the 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"

Fixed!

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

Ok! Removed `inline` from the two new static helper functions here.

>> Source/WebCore/platform/ios/PasteboardIOS.mm:260
>> +        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.

Sounds good — removed the local variable.

>> Source/WebCore/platform/ios/PasteboardIOS.mm:278
>> +        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;

Removed the local variable and changed the last part to be a single return statement.

[Wenson Hsieh]

Created attachment 366399 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366399 [details]
Patch for landing

Clearing flags on attachment: 366399

Committed r243695: <https://trac.webkit.org/changeset/243695>