[webkit-reviews] review granted: [Bug 173634] [iOS] Cannot italicize or bold text rendered with text styles : [Attachment 313472] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 173634: [iOS] Cannot italicize or bold text rendered with text styles
https://bugs.webkit.org/show_bug.cgi?id=173634

Attachment 313472: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 313472
  --> https://bugs.webkit.org/attachment.cgi?id=313472
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(*thi
s, 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.