Bug 147483 – [OS X] Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage]

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

[Myles C. Maxfield]

Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage]

[Myles C. Maxfield]

Created attachment 257900 [details]
Patch

[Myles C. Maxfield]

Created attachment 257902 [details]
Patch

[Build Bot]

Comment on attachment 257902 [details]
Patch

Attachment 257902 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1844

New failing tests:
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/svg-fonts-without-missing-glyph.xhtml
svg/batik/text/xmlSpace.svg
fast/css/font-face-opentype.html
svg/custom/svg-fonts-fallback.xhtml

[Build Bot]

Created attachment 257905 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 257902 [details]
Patch

Attachment 257902 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1891

New failing tests:
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/svg-fonts-without-missing-glyph.xhtml
svg/batik/text/xmlSpace.svg
fast/css/font-face-opentype.html
svg/custom/svg-fonts-fallback.xhtml

[Build Bot]

Created attachment 257906 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Failures do not occur on Yosemite

[Myles C. Maxfield]

Created attachment 257912 [details]
Patch

[Build Bot]

Comment on attachment 257912 [details]
Patch

Attachment 257912 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2187

New failing tests:
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/svg-fonts-without-missing-glyph.xhtml
svg/batik/text/xmlSpace.svg
fast/css/font-face-opentype.html
svg/custom/svg-fonts-fallback.xhtml

[Build Bot]

Created attachment 257914 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 257912 [details]
Patch

Attachment 257912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2191

New failing tests:
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/svg-fonts-without-missing-glyph.xhtml
svg/batik/text/xmlSpace.svg
fast/css/font-face-opentype.html
svg/custom/svg-fonts-fallback.xhtml

[Build Bot]

Created attachment 257915 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Myles C. Maxfield]

Comment on attachment 257912 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:56
> +@interface NSFont (Private2)
> ++ (NSFont *)findFontLike:(NSFont *)aFont forCharacter:(UInt32)c inLanguage:(id) language;
> ++ (NSFont *)findFontLike:(NSFont *)aFont forString:(NSString *)string withRange:(NSRange)range inLanguage:(id) language;
> +
> ++ (NSFont *)systemFontOfSize:(CGFloat)size weight:(CGFloat)weight;
> +@end

These need to go away.

[Myles C. Maxfield]

Comment on attachment 257912 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:505
> +    if (!CFEqual(substituteFont, substituteFont1)) {
> +        WTFLogAlways("URLs: %s %s", [(NSString*)CFURLGetString((CFURLRef)CTFontCopyAttribute((CTFontRef)substituteFont, kCTFontReferenceURLAttribute)) UTF8String], [(NSString*)CFURLGetString((CFURLRef)CTFontCopyAttribute((CTFontRef)substituteFont1, kCTFontReferenceURLAttribute)) UTF8String]);
> +        WTFLogAlways("Names: %s %s", [(NSString*)CTFontCopyAttribute((CTFontRef)substituteFont, kCTFontNameAttribute) UTF8String], [(NSString*)CTFontCopyAttribute((CTFontRef)substituteFont1, kCTFontNameAttribute) UTF8String]);
> +    }

Woah debugging no no no

[Myles C. Maxfield]

Created attachment 257989 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 257989 [details]
Patch

Dino r+'ed this

[Myles C. Maxfield]

Committed r187707 http://trac.webkit.org/changeset/187707

[WebKit Commit Bot]

Re-opened since this is blocked by bug 147617

[Myles C. Maxfield]

Test failure reproduces consistently

[Myles C. Maxfield]

Created attachment 258268 [details]
This time with 100% less test flakiness!

[Myles C. Maxfield]

Committed r187982: <http://trac.webkit.org/changeset/187982>

[Darin Adler]

Comment on attachment 258268 [details]
This time with 100% less test flakiness!

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:471
> +    Vector<RetainPtr<CTFontRef>> toRemove;

This should just be a Vector of raw pointers, not of RetainPtr. There is no value in doing retain/release here since the fallbackDedupSet already retains the fonts that are in it.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:482
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED == 1090

Using == here seems like a mistake. Do you mean >=? Or maybe <=?

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:484
> +        font = reinterpret_cast<CTFontRef>([NSFont userFontOfSize:fontSize]);

I suggest we make inline functions for these casts. It’s too hard to see if these are correct at the call site, but we could just make inline functions that take and return the appropriate types. Those functions can even include comments explaining why the casts are correct.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:495
> +        bool acceptable = true;
> +        
> +        RetainPtr<CFCharacterSetRef> characterSet = adoptCF(CTFontCopyCharacterSet(font));
> +        for (auto character : StringView(characters, length).codePoints()) {
> +            if (!CFCharacterSetIsLongCharacterMember(characterSet.get(), character)) {
> +                acceptable = false;
> +                break;
> +            }
> +        }
> +        if (acceptable)
> +            return font;

Is this for correctness or for speed? Is this an important optimization? Won’t CTFontCreateForCharactersWithLanguage do this?

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:498
> +    CFIndex coveredLength = 0;

Is initializing this to 0 valuable? Isn’t it just an out argument?

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:513
> +    NSFont *substituteFont = reinterpret_cast<NSFont *>(const_cast<__CTFont*>(fallbackDedupSet().add(result).iterator->get()));

I suggest we make inline functions for these casts. It’s too hard to see if these are correct at the call site, but we could just make inline functions that take and return the appropriate types. Those functions can even include comments explaining why the casts are correct.

> Source/WebCore/platform/spi/mac/NSFontSPI.h:37
>  @interface NSFont (Private)

I think best practice is to put () instead of (Private) in places like this.