Bug 147483

Summary: [OS X] Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage]
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dino, jonlee, rniwa, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 147457, 147617    
Bug Blocks: 147390    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Patch
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
This time with 100% less test flakiness! dino: review+

Description Myles C. Maxfield 2015-07-30 22:11:18 PDT
Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage]
Comment 1 Myles C. Maxfield 2015-07-30 22:17:29 PDT
Created attachment 257900 [details]
Patch
Comment 2 Myles C. Maxfield 2015-07-30 22:33:47 PDT
Created attachment 257902 [details]
Patch
Comment 3 Build Bot 2015-07-30 23:07:55 PDT
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
Comment 4 Build Bot 2015-07-30 23:07:58 PDT
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
Comment 5 Build Bot 2015-07-30 23:31:34 PDT
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
Comment 6 Build Bot 2015-07-30 23:31:38 PDT
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
Comment 7 Myles C. Maxfield 2015-07-31 00:40:09 PDT
Failures do not occur on Yosemite
Comment 8 Myles C. Maxfield 2015-07-31 00:40:59 PDT
Created attachment 257912 [details]
Patch
Comment 9 Build Bot 2015-07-31 01:16:00 PDT
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
Comment 10 Build Bot 2015-07-31 01:16:06 PDT
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
Comment 11 Build Bot 2015-07-31 01:20:12 PDT
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
Comment 12 Build Bot 2015-07-31 01:20:15 PDT
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
Comment 13 Myles C. Maxfield 2015-07-31 16:00:29 PDT
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.
Comment 14 Myles C. Maxfield 2015-07-31 16:01:29 PDT
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
Comment 15 Myles C. Maxfield 2015-07-31 19:22:55 PDT
Created attachment 257989 [details]
Patch
Comment 16 Myles C. Maxfield 2015-07-31 19:29:12 PDT
Comment on attachment 257989 [details]
Patch

Dino r+'ed this
Comment 17 Myles C. Maxfield 2015-07-31 21:07:08 PDT
Committed r187707 http://trac.webkit.org/changeset/187707
Comment 18 WebKit Commit Bot 2015-08-03 19:59:45 PDT
Re-opened since this is blocked by bug 147617
Comment 19 Myles C. Maxfield 2015-08-04 22:17:55 PDT
Test failure reproduces consistently
Comment 20 Myles C. Maxfield 2015-08-04 23:57:34 PDT
Created attachment 258268 [details]
This time with 100% less test flakiness!
Comment 21 Myles C. Maxfield 2015-08-05 13:07:57 PDT
Committed r187982: <http://trac.webkit.org/changeset/187982>
Comment 22 Darin Adler 2015-08-08 14:20:20 PDT
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.