Bug 215707 – [iOS] attachmentActionFont() Needs to use kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) to get the short emphasized footnote font

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[iOS] CTFontDescriptorCreateWithTextStyleAndAttributes() is faster than CTFontDescriptorCreateWithTextStyle()/CTFontDescriptorCreateCopyWithAttributes()

[Myles C. Maxfield]

Created attachment 406949 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67887880>

[Myles C. Maxfield]

Created attachment 408079 [details]
Patch

[Sam Weinig]

Comment on attachment 408079 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        [iOS] CTFontDescriptorCreateWithTextStyleAndAttributes() is faster than CTFontDescriptorCreateWithTextStyle()/CTFontDescriptorCreateCopyWithAttributes()
> +        https://bugs.webkit.org/show_bug.cgi?id=215707

Do you have any perf measurements for this claim?

> Source/WTF/wtf/PlatformHave.h:740
> +#define HAVE_CTFONTDESCRIPTORCREATEWITHTEXTSTYLEANDATTRIBUTES 1

Please throw some underscores in there. This is not really readable as is.

[Darin Adler]

Comment on attachment 408079 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1403
> +    RetainPtr<CTFontDescriptorRef> emphasizedFontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyleAndAttributes(kCTUIFontTextStyleFootnote, RenderThemeIOS::contentSizeCategory(), (CFDictionaryRef)@{ (id)kCTFontTraitsAttribute: @{ (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) } }));

auto

[Myles C. Maxfield]

<rdar://problem/63930892>

[Myles C. Maxfield]

Oh, I see. Even if CTFontDescriptorCreateWithTextStyleAndAttributes() doesn’t exist, we should still modify the existing code to use (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized). This is about correctness, not performance.

[Myles C. Maxfield]

Created attachment 408147 [details]
Patch

[Darin Adler]

Comment on attachment 408147 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1408
> +#if HAVE(CTFONTDESCRIPTOR_CREATE_WITH_TEXT_STYLE_AND_ATTRIBUTES)
> +    auto emphasizedFontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyleAndAttributes(kCTUIFontTextStyleFootnote, RenderThemeIOS::contentSizeCategory(), (CFDictionaryRef)@{ (id)kCTFontTraitsAttribute: @{ (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) } }));
> +#else
> +    auto fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(kCTUIFontTextStyleShortFootnote, RenderThemeIOS::singleton().contentSizeCategory(), 0));
> +    auto emphasizedFontDescriptor = adoptCF(CTFontDescriptorCreateCopyWithAttributes(fontDescriptor.get(),
> +        (CFDictionaryRef)@{ (id)kCTFontTraitsAttribute: @{ (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) } }));
> +#endif

Maybe we could put the attributes dictionary outside the #if? I wouldn’t mind having one more local variable for it.

[Myles C. Maxfield]

Created attachment 408151 [details]
Patch

[Darin Adler]

Comment on attachment 408151 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1403
> +    auto emphasizedFontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyleAndAttributes(kCTUIFontTextStyleFootnote, RenderThemeIOS::contentSizeCategory(), (CFDictionaryRef)@{ (id)kCTFontTraitsAttribute: @{ (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) } }));

This calls RenderThemeIOS::contentSizeCategory().

> Source/WebCore/rendering/RenderThemeIOS.mm:1405
> +    auto fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(kCTUIFontTextStyleShortFootnote, RenderThemeIOS::singleton().contentSizeCategory(), 0));

This calls RenderThemeIOS::singleton().contentSizeCategory().

Why the difference?

I suggest refactoring so only the CTFontDescriptorCreateWithTextStyleAndAttributes call has an #if in it, and all the arguments are shared.

[Myles C. Maxfield]

Created attachment 408152 [details]
Patch

[Myles C. Maxfield]

Created attachment 408155 [details]
Patch

[Darin Adler]

Comment on attachment 408155 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:1404
> +    CFStringRef style = kCTUIFontTextStyleFootnote;
> +    CFStringRef size = RenderThemeIOS::singleton().contentSizeCategory();
> +    CFDictionaryRef attributes = static_cast<CFDictionaryRef>(@{ (id)kCTFontTraitsAttribute: @{ (id)kCTFontSymbolicTrait: @(kCTFontTraitTightLeading | kCTFontTraitEmphasized) } });

I would have used auto for all three of these.

[Myles C. Maxfield]

Created attachment 408157 [details]
Patch for committing

[EWS]

Committed r266693: <https://trac.webkit.org/changeset/266693>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408157 [details].