Bug 167056 – [Cocoa] Unify font fallback between macOS and iOS for when the font-family list is exhausted

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

[Myles C. Maxfield]

[Cocoa] Unify font fallback between macOS and iOS for when the font-family list is exhausted

[Myles C. Maxfield]

Created attachment 298860 [details]
Patch

[Myles C. Maxfield]

Created attachment 298868 [details]
Patch

[Darin Adler]

Comment on attachment 298868 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:880
> +    RetainPtr<CTFontRef> result = adoptCF(CTFontCreateForCharactersWithLanguage(font, characters, length, localeString.get(), &coveredLength));

This is one place where I really like auto. Like below when we do the CTFontCopyAttribute.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:885
> +    UChar32 c = *characters;
> +    if (length > 1 && U16_IS_LEAD(c) && U16_IS_TRAIL(characters[1]))
> +        c = U16_GET_SUPPLEMENTARY(c, characters[1]);

This code is just something you are moving, but I have some thoughts on it.

This code assumes that length is not zero. We should assert that, I think, maybe at the top of the function.

I suggest we name this variable "firstCharacter" instead of "c". But also, if we wanted to do this, we should use the ICU macro that exists for this whole operation instead of writing out the algorithm:

    UChar32 firstCharacter;
    U16_GET(characters, 0, 0, length, firstCharacter);

And further, the only thing we do with this character is check that it’s in the range 0x600-0x6FF, so there is no point on including all the surrogate logic. It can just be more like this:

    UChar firstCharacter = characters[0];

Could use a “why” comment about why it’s OK to only look at the first character.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:887
> +    // Arabic
> +    if (c >= 0x0600 && c <= 0x06ff) {

I personally prefer capitalized hex. Also might make this slightly more self documenting if we encapsulated this check in an isArabicCharacter function. We would not need that comment.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:893
> +            RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

auto here

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:894
> +            RetainPtr<CTFontDescriptorRef> modification = adoptCF(CTFontDescriptorCreateWithAttributes(attributes.get()));

auto here

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:917
> +    RetainPtr<CTFontRef> result = lookupFallbackFont(platformData.font(), description.weight(), description.locale(), characters, length);

auto here

[Myles C. Maxfield]

Created attachment 298884 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 298884 [details]
Patch for committing

Clearing flags on attachment: 298884

Committed r210776: <http://trac.webkit.org/changeset/210776>