[Cocoa] Unify FontCache
Created attachment 259716 [details] Patch
Created attachment 259718 [details] Patch
Comment on attachment 259718 [details] Patch Attachment 259718 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/89434 New failing tests: fast/text/weighted-italicized-system-font.html
Created attachment 259720 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 259718 [details] Patch Attachment 259718 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/89525 New failing tests: fast/text/weighted-italicized-system-font.html
Created attachment 259721 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 259718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259718&action=review > Source/WebCore/platform/graphics/FontCache.h:222 > +FontWeight fontWeightfromCT(CGFloat weight); Strange name. Strong to have “from” all lowercase in a name that’s mostly using intercaps. Also strange to use CT in this name and then use CoreText in the name of the function just below! > Source/WebCore/platform/graphics/FontCache.h:226 > +typedef std::pair<bool, bool> SynthesisPair; // synthetic bold, synthetic italic We normally put typedefs first, rather than mixed in with functions. > Source/WebCore/platform/graphics/FontCache.h:231 > +bool requiresCustomFallbackFont(const UInt32 character); The const here has no value so it should be omitted; we don’t mark all our int arguments const, although we could. I think the type here should be UChar32, not UInt32, even though they are roughly the same thing, but I could be wrong about the types used in CoreText functions. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:187 > if (weight < -0.6) > - weightMask = FontWeight100Mask; > + return FontWeight100; > else if (weight < -0.365) > - weightMask = FontWeight200Mask; > + return FontWeight200; > else if (weight < -0.115) > - weightMask = FontWeight300Mask; > + return FontWeight300; > else if (weight < 0.130) > - weightMask = FontWeight400Mask; > + return FontWeight400; > else if (weight < 0.235) > - weightMask = FontWeight500Mask; > + return FontWeight500; > else if (weight < 0.350) > - weightMask = FontWeight600Mask; > + return FontWeight600; > else if (weight < 0.500) > - weightMask = FontWeight700Mask; > + return FontWeight700; > else if (weight < 0.700) > - weightMask = FontWeight800Mask; > + return FontWeight800; > else > + return FontWeight900; Can delete all those “elses”. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:354 > + return fontFamilies; return { }; > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:358 > + return fontFamilies; return { }; > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368 > + copyToVector(visited, fontFamilies); > + return fontFamilies; Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:381 > +SynthesisPair computeNecessarySynthesis(CTFontRef font, const FontDescription& fontDescription, bool isPlatformFont) The use of pair here is really unclear. A struct with two bool would probably be better. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:385 > + return std::make_pair(false, false); Could just say: return { false, false}; or event: return { }; > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:389 > + return std::make_pair(false, false); Ditto. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:398 > + bool syntheticBold = (fontDescription.fontSynthesis() & FontSynthesisWeight) && (desiredTraits & kCTFontTraitBold) && !(actualTraits & kCTFontTraitBold); > + bool syntheticOblique = (fontDescription.fontSynthesis() & FontSynthesisStyle) && (desiredTraits & kCTFontTraitItalic) && !(actualTraits & kCTFontTraitItalic); I would suggest considering different names here. The variable itself does not contain “synthetic bold”, instead it is a flag that says, “should synthesize bold” or “need synthetic bold” or something like that. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:400 > + return std::make_pair(syntheticBold, syntheticOblique); Can do: return { syntheticBold, syntheticOblique }; Or: return { (fontDescription.fontSynthesis() & FontSynthesisWeight) && (desiredTraits & kCTFontTraitBold) && !(actualTraits & kCTFontTraitBold), (fontDescription.fontSynthesis() & FontSynthesisStyle) && (desiredTraits & kCTFontTraitItalic) && !(actualTraits & kCTFontTraitItalic) }; > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422 > + if (whitelist.size() && !whitelist.contains(family.lower())) No need for lower() here since the HashSet uses CaseFoldingHash. It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:425 > + RetainPtr<CTFontRef> foundFont = adoptCF(CTFontCreateForCSS(family.string().createCFString().get(), toCoreTextFontWeight(weight), requestedTraits, size)); I suggest using auto here. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:478 > + RetainPtr<CTFontRef> autoActivatedFont = adoptCF(CTFontCreateWithName(family.string().createCFString().get(), size, nullptr)); Not sure exactly why we put this into a local variable if we want to ignore it. Is there some better idiom for this? > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:512 > +RefPtr<Font> FontCache::systemFallbackForCharacters(const FontDescription& description, const Font* originalFontData, bool isPlatformFont, const UChar* characters, unsigned length) I think this cannot return null and thus is should return Ref, not RefPtr. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532 > + CTFontRef substituteFont = fallbackDedupSet().add(result).iterator->get(); Might save a little reference count churn here by using WTF::move(result); not sure. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:171 > +static inline FontWeight appkitWeightToFontWeight(NSInteger appKitWeight) > +{ > + return appKitWeight == 1 ? FontWeight100 : > + appKitWeight == 2 ? FontWeight200 : > + appKitWeight <= 4 ? FontWeight300 : > + appKitWeight == 5 ? FontWeight400 : > + appKitWeight == 6 ? FontWeight500 : > + appKitWeight <= 8 ? FontWeight600 : > + appKitWeight == 9 ? FontWeight700 : > + appKitWeight <= 11 ? FontWeight800 : > + FontWeight900; > +} Why is the coding style of this function so different from fontWeightfromCT; they are both doing the same kind of job. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:228 > + RetainPtr<CFDictionaryRef> featureIdentifier = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, featureKeys, featureValues, WTF_ARRAY_LENGTH(featureKeys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); > + RetainPtr<CFDictionaryRef> featureIdentifierPtr = featureIdentifier.get(); > + RetainPtr<CFArrayRef> featureArray = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&featureIdentifierPtr, 1, &kCFTypeArrayCallBacks)); Why is the type of featureIdentifierPtr RetainPtr<CFDictionaryRef>? I think it should be CFTypeRef and then I think we would not need any type casting and we would not do extra reference count churn. As the code currently is, there is no reason to use featureIdentifierPtr instead of featureIdentifier in the CFArrayCreate call.! > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:410 > + static NeverDestroyed<AtomicString> timesStr("Times", AtomicString::ConstructFromLiteral); I’m not sure this optimization is helpful and needed. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:422 > + static NeverDestroyed<AtomicString> lucidaGrandeStr("Lucida Grande", AtomicString::ConstructFromLiteral); I’m not sure this optimization is helpful and needed.
Created attachment 259746 [details] Patch
Comment on attachment 259718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259718&action=review >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368 >> + return fontFamilies; > > Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe. I think this is a good idea to do in a follow-up patch. >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422 >> + if (whitelist.size() && !whitelist.contains(family.lower())) > > No need for lower() here since the HashSet uses CaseFoldingHash. > > It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all. How would you feel if I made the Whitelist an Optional<HashSet>? >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:478 >> + RetainPtr<CTFontRef> autoActivatedFont = adoptCF(CTFontCreateWithName(family.string().createCFString().get(), size, nullptr)); > > Not sure exactly why we put this into a local variable if we want to ignore it. Is there some better idiom for this? The variable is necessary because of the WARN_UNUSED_RESULT. A better solution is to forgo the RetainPtr entirely and just use CFRelease(CTFontCreateWithName()). >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:512 >> +RefPtr<Font> FontCache::systemFallbackForCharacters(const FontDescription& description, const Font* originalFontData, bool isPlatformFont, const UChar* characters, unsigned length) > > I think this cannot return null and thus is should return Ref, not RefPtr. It can return nullptr on other ports (see FontCacheFreeType.cpp). >> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532 >> + CTFontRef substituteFont = fallbackDedupSet().add(result).iterator->get(); > > Might save a little reference count churn here by using WTF::move(result); not sure. I don't want to move from the dedupSet; that's the whole point of it (unless I am misunderstanding you). >> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:228 >> + RetainPtr<CFArrayRef> featureArray = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&featureIdentifierPtr, 1, &kCFTypeArrayCallBacks)); > > Why is the type of featureIdentifierPtr RetainPtr<CFDictionaryRef>? I think it should be CFTypeRef and then I think we would not need any type casting and we would not do extra reference count churn. > > As the code currently is, there is no reason to use featureIdentifierPtr instead of featureIdentifier in the CFArrayCreate call.! Good catch!
Created attachment 259749 [details] Patch
Comment on attachment 259749 [details] Patch r=me
Comment on attachment 259718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259718&action=review >>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368 >>> + return fontFamilies; >> >> Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe. > > I think this is a good idea to do in a follow-up patch. https://bugs.webkit.org/show_bug.cgi?id=148383 >>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422 >>> + if (whitelist.size() && !whitelist.contains(family.lower())) >> >> No need for lower() here since the HashSet uses CaseFoldingHash. >> >> It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all. > > How would you feel if I made the Whitelist an Optional<HashSet>? https://bugs.webkit.org/show_bug.cgi?id=148382
Committed r188871: <http://trac.webkit.org/changeset/188871>
Small build fix in http://trac.webkit.org/changeset/188874
And https://trac.webkit.org/changeset/188875, which I'm less sure of. Myles, can you check?
http://trac.webkit.org/changeset/188877 fixes these up.