Summary: | [OS X] Migrate to CTFontCreateForCharactersWithLanguage from [NSFont findFontLike:forString:withRange:inLanguage] | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Myles C. Maxfield
2015-07-30 22:11:18 PDT
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
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. |