Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage]
Created attachment 257900 [details] Patch
Created attachment 257902 [details] Patch
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
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 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
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
Failures do not occur on Yosemite
Created attachment 257912 [details] Patch
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
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 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
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 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 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
Created attachment 257989 [details] Patch
Comment on attachment 257989 [details] Patch Dino r+'ed this
Committed r187707 http://trac.webkit.org/changeset/187707
Re-opened since this is blocked by bug 147617
Test failure reproduces consistently
Created attachment 258268 [details] This time with 100% less test flakiness!
Committed r187982: <http://trac.webkit.org/changeset/187982>
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.