Bug 215707

Summary:

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

Product:

WebKit

Reporter:

Myles C. Maxfield <mmaxfield>

Component:

New Bugs

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, changseok, cmarcelo, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, sam, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=155485

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	darin: review+
Patch for committing	none

Description Myles C. Maxfield 2020-08-20 11:47:40 PDT

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

Comment 1 Myles C. Maxfield 2020-08-20 11:54:45 PDT

Created attachment 406949 [details]
Patch

Comment 2 Radar WebKit Bug Importer 2020-08-27 11:48:13 PDT

<rdar://problem/67887880>

Comment 3 Myles C. Maxfield 2020-09-05 02:09:34 PDT

Created attachment 408079 [details]
Patch

Comment 4 Sam Weinig 2020-09-05 08:58:58 PDT

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.

Comment 5 Darin Adler 2020-09-05 09:05:34 PDT

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

Comment 6 Myles C. Maxfield 2020-09-06 15:44:37 PDT

<rdar://problem/63930892>

Comment 7 Myles C. Maxfield 2020-09-06 15:49:18 PDT

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.

Comment 8 Myles C. Maxfield 2020-09-06 15:59:14 PDT

Created attachment 408147 [details]
Patch

Comment 9 Darin Adler 2020-09-06 16:48:37 PDT

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.

Comment 10 Myles C. Maxfield 2020-09-06 17:02:22 PDT

Created attachment 408151 [details]
Patch

Comment 11 Darin Adler 2020-09-06 17:06:30 PDT

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.

Comment 12 Myles C. Maxfield 2020-09-06 17:07:25 PDT

Created attachment 408152 [details]
Patch

Comment 13 Myles C. Maxfield 2020-09-06 17:37:36 PDT

Created attachment 408155 [details]
Patch

Comment 14 Darin Adler 2020-09-06 17:59:05 PDT

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.

Comment 15 Myles C. Maxfield 2020-09-06 18:06:01 PDT

Created attachment 408157 [details]
Patch for committing

Comment 16 EWS 2020-09-06 23:19:42 PDT

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

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