Bug 173634 – [iOS] Cannot italicize or bold text rendered with text styles

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[iOS] Cannot italicize or bold text rendered with text styles

[Myles C. Maxfield]

Created attachment 313468 [details]
Patch

[Myles C. Maxfield]

Created attachment 313472 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/31805407>

[Darin Adler]

Comment on attachment 313472 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:38
> +#if PLATFORM(IOS)
> +#include "RenderThemeIOS.h"
> +#endif

Maybe #if USE_PLATFORM_SYSTEM_FALLBACK_LIST instead?

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:110
> +    enum class ClientUse {
> +        ForSystemUI,
> +        ForTextStyle
> +    };

I’d define this on one line.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:196
> +static inline bool isTextStyleString(const AtomicString& string)

I don’t really find the name here satisfying. The words “text style” alone in WebKit code, even in this source file, don’t mean “UIFontTextStyle” to mean. I think this function should be named isUIFontTextStyle or something else like that. Or maybe you should say out loud what question this answers and figure out from that what the name should be. Are these “text styles” special font names? Or special font family names? That could drive what name the function should have.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:240
> +    static NeverDestroyed<AtomicString> UIFontTextStyleHeadline(kCTUIFontTextStyleHeadline);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleBody(kCTUIFontTextStyleBody);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTitle1(kCTUIFontTextStyleTitle1);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTitle2(kCTUIFontTextStyleTitle2);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTitle3(kCTUIFontTextStyleTitle3);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleSubhead(kCTUIFontTextStyleSubhead);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleFootnote(kCTUIFontTextStyleFootnote);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleCaption1(kCTUIFontTextStyleCaption1);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleCaption2(kCTUIFontTextStyleCaption2);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleShortHeadline(kCTUIFontTextStyleShortHeadline);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleShortBody(kCTUIFontTextStyleShortBody);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleShortSubhead(kCTUIFontTextStyleShortSubhead);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleShortFootnote(kCTUIFontTextStyleShortFootnote);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleShortCaption1(kCTUIFontTextStyleShortCaption1);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTallBody(kCTUIFontTextStyleTallBody);
> +    static NeverDestroyed<AtomicString> FontDescriptorTextStyleEmphasized(kCTFontDescriptorTextStyleEmphasized);
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTitle0(kCTUIFontTextStyleTitle0);
> +    static NeverDestroyed<AtomicString> UIFontTextStyleTitle4(kCTUIFontTextStyleTitle4);
> +#endif
> +
> +    return string == UIFontTextStyleHeadline.get()
> +        || string == UIFontTextStyleBody.get()
> +        || string == UIFontTextStyleTitle1.get()
> +        || string == UIFontTextStyleTitle2.get()
> +        || string == UIFontTextStyleTitle3.get()
> +        || string == UIFontTextStyleSubhead.get()
> +        || string == UIFontTextStyleFootnote.get()
> +        || string == UIFontTextStyleCaption1.get()
> +        || string == UIFontTextStyleCaption2.get()
> +        || string == UIFontTextStyleShortHeadline.get()
> +        || string == UIFontTextStyleShortBody.get()
> +        || string == UIFontTextStyleShortSubhead.get()
> +        || string == UIFontTextStyleShortFootnote.get()
> +        || string == UIFontTextStyleShortCaption1.get()
> +        || string == UIFontTextStyleTallBody.get()
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000
> +        || string == FontDescriptorTextStyleEmphasized.get()
> +        || string == UIFontTextStyleTitle0.get()
> +        || string == UIFontTextStyleTitle4.get();
> +#else
> +        || string == FontDescriptorTextStyleEmphasized.get();
> +#endif

This should be done with arrays so we don’t have to list everything twice and worry about whether we forgot something, and so we generate smaller code too; unrolled loops are big. And I also suggest we sort alphabetically within each #if so it’s easier to see if something is missing rather than using logical order or an order that matches a header file. Note we can use a trailing comma on the last item so there is no need to be tricky about the last line.

We can start with this convertArray function, which is generic for any case where we want to use the assignment operator to convert from one type to another. That works fine for CFStringRef to AtomicString:

template<typename T, typename U, std::size_t size> inline std::array<T, size> convertArray(U (&array)[size])
{
    std::array<T, size> result;
    for (size_t i = 0; i < size; ++i)
        result[i] = array[i];
    return result;
}

If all our compilers supported C++14 well enough that we could use those features, we could write this better version that uses construction rather than assignment:

template< typename T, typename U, std::size_t size, std::size_t... indices> std::array<T, size> convertArray(U (&array)[size], std::index_sequence<indices...>)
{
    return { { array[indices]... } };
}

template<typename T, typename U, std::size_t size> inline std::array<T, size> convertArray(U (&array)[size])
{
    return convertArray<T>(array, std::make_index_sequence<size> { });
}

This is like the experimental standard to_array function, but works when the types don’t match. Once we have convertArray, we can write the code like this:

    static const CFStringRef styles[] = {
        kCTFontDescriptorTextStyleEmphasized,
        kCTUIFontTextStyleBody,
        ...
        kCTUIFontTextStyleTitle3,
#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000
        kCTUIFontTextStyleTitle0,
        kCTUIFontTextStyleTitle4,
#endif
    };
    static const NeverDestroyed<std::array<AtomicString, WTF_ARRAY_LENGTH(styles)>> strings { convertArray<AtomicString>(styles) };
    return std::find(strings.get().begin(), strings.get().end(), string) != strings.get().end();

If it wasn’t for the need for NeverDestroyed, we could use just "const auto" as the type for "strings", which makes this code look even better.

We could even go further and use std::binary_search instead of std::find, but it’s slightly tricky because we want an operator< predicate that would compares two AtomicStrings based on their pointer values rather than on the values of the strings. We'd have to pass that predicate to the convertToSortedArray function so it can sort when creating the array, and also pass it to binary_search; so the code gets a little bit wordy. But if the list of styles was long enough I suppose we would want to do that for better performance.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:273
> +        static NeverDestroyed<AtomicString> systemUI = AtomicString("system-ui", AtomicString::ConstructFromLiteral);
> +        result.fontName = systemUI.get();

This idiom is so wordy. For the sake of future programming I’d really like to understand whether the optimization is important enough to be worth it. This comes up a lot when programming on WebKit so I would love to have a firm idea how to make the tradeoff. When is this OK?

    result.fontName = "system-ui";

And when do we have to go further? ASCIILiteral, AtomicString::ConstructFromLiteral, NeverDestroyed, etc. In addition, if we are going to use that ConstructFromLiteral style a lot, then we should probably make a helper function so it’s not so ugly. Unless we can’t use that function without making the code worse.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:294
> +            result += SystemFontDatabase::singleton().systemFontCascadeList(systemFontParameters(*this, cssFamily, SystemFontDatabase::ClientUse::ForSystemUI), SystemFontDatabase::ClientUse::ForSystemUI).size();

This gets so long I think we should make a helper function. Then we wouldn’t have to repeat SystemFontDatabase::ClientUse::ForSystemUI twice, for example. Could use that helper function four times.

[Darin Adler]

Here’s a better version. If we add this function:

template<typename T> inline NeverDestroyed<T> makeNeverDestroyed(T&& argument)
{
    return WTFMove(argument);
}

Then the line that defines "strings" can be this instead:

    static const auto strings { makeNeverDestroyed(convertArray<AtomicString>(styles)) };

Probably best to put makeNeverDestroyed into NeverDestroyed.h, but it can also just be in a file where it’s used at first and go into NeverDestroyed.h later. We could use this to clean up any place that uses NeverDestroyed to use auto or const auto for the type rather than writing it out.

[Myles C. Maxfield]

Comment on attachment 313472 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:38
>> +#endif
> 
> Maybe #if USE_PLATFORM_SYSTEM_FALLBACK_LIST instead?

Text styles are iOS-specific.

>> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:273
>> +        result.fontName = systemUI.get();
> 
> This idiom is so wordy. For the sake of future programming I’d really like to understand whether the optimization is important enough to be worth it. This comes up a lot when programming on WebKit so I would love to have a firm idea how to make the tradeoff. When is this OK?
> 
>     result.fontName = "system-ui";
> 
> And when do we have to go further? ASCIILiteral, AtomicString::ConstructFromLiteral, NeverDestroyed, etc. In addition, if we are going to use that ConstructFromLiteral style a lot, then we should probably make a helper function so it’s not so ugly. Unless we can’t use that function without making the code worse.

I don't have any particular thoughts on the way this should be; I'm happy to follow whatever the generally-accepted best practices are.

result.fontName = "system-ui"; is bad because it causes an allocation.
result.fontName = AtomicString("system-ui", AtomicString::ConstructFromLiteral); isn't bad, but isn't great, because it still has to hash the characters to find the preexisting String
result.fontName = ASCIILiteral("system-ui"); AFAIK has the same tradeoffs as the previous one.
result.fontName = systemUI.get(); is good because copying an AtomicString is just bumping its reference count.

[Myles C. Maxfield]

Created attachment 313973 [details]
Patch for committing

[Myles C. Maxfield]

Committed r218909: <http://trac.webkit.org/changeset/218909>