[OS X] Use CTFontCreateForCSS instead of doing font search ourselves
Created attachment 230124 [details] Patch
Created attachment 230231 [details] Patch
Diff wasn't able to properly inspect LayoutTests/fast/table/frame-and-rules.html. the difference there is that I updated the test to not try to search for a font named "italic" but instead to search for "times"
Created attachment 230232 [details] Without frame-and-rules.html
Comment on attachment 230232 [details] Without frame-and-rules.html View in context: https://bugs.webkit.org/attachment.cgi?id=230232&action=review I'm not fully sure about the validity of your solution. The code you removed in DRT was forcing the tests to use fonts for the list of supported appkit fonts, therefore making sure WebKit was using fallback for anything other than the fonts that were in the installed list. This guarantees consistent results regardless of the fonts that are installed on a particular system. I think you are losing this piece of functionality with your patch. > Source/WebCore/ChangeLog:8 > + This tests migrates to using a CoreText function to perform font lookup Test? You probably meant patch.
Yep. That's by design. Turns out that almost none of our tests rely on this behavior, and this patch fixes up those tests. Is that a deal breaker? (In reply to comment #5) > (From update of attachment 230232 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230232&action=review > > I'm not fully sure about the validity of your solution. The code you removed in DRT was forcing the tests to use fonts for the list of supported appkit fonts, therefore making sure WebKit was using fallback for anything other than the fonts that were in the installed list. This guarantees consistent results regardless of the fonts that are installed on a particular system. I think you are losing this piece of functionality with your patch. > > > Source/WebCore/ChangeLog:8 > > + This tests migrates to using a CoreText function to perform font lookup > > Test? You probably meant patch.
Comment on attachment 230232 [details] Without frame-and-rules.html The "limit the available" fonts code in DRT was added for a reason: tests had different results on different machines because they were sensitive to the set of installed fonts. We should not regress this behavior.
<rdar://problem/16718861>
Putting this on the back burner for now, as the current NSFontManager path works (it's just a maintenance burden)
Created attachment 236497 [details] Patch
Created attachment 239597 [details] Patch
Created attachment 241757 [details] Patch
CTFontCreateForCSS will never return a screen font. However, all our testing assumes that ::only:: screen fonts are used. I am going to migrate all our tests to using printer fonts. Once I do that, this patch can go in.
Created attachment 250546 [details] Patch
Comment on attachment 250546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250546&action=review I see quite a few small mistakes; this also overuses sets. But generally looks fine, so r=me assuming you fix it. > Source/WebCore/platform/graphics/FontCache.h:123 > + WEBCORE_EXPORT static void setFontWhitelist(const HashSet<String>&); Should take a Vector, not a HashSet. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:44 > +#import <wtf/HashSet.h> Should not be needed since we are including this in FontCache.h. Except that we probably shouldn’t have it in FontCache.h any more. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:114 > +#endif Should add a blank line before this to match above formatting. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:161 > +static HashSet<String>& fontWhitelist() > +{ > + static NeverDestroyed<HashSet<String>> whitelist; > + return whitelist; > +} We want this to be case insensitive, so we should do that, and use HashSet<String, CaseFoldingHash>, and not call lower() all the time before using the hash table. Avoids unnecessary memory allocation. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:163 > +void FontCache::setFontWhitelist(const HashSet<String>& whitelist) This should take a vector of strings, not a HashSet. Or if it does take a HashSet then it should just do assignment, not a loop, but it would need to be HashSet<String, CaseFoldingHash>. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:166 > + HashSet<String>& gWhitelist = fontWhitelist(); Should not use the "g" prefix here. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:178 > + if (equalIgnoringCase(family, "-webkit-system-font") > + || equalIgnoringCase(family, "-apple-system-font")) { Should merge this into a single line. Also would be good to be "ignoring ASCII case", rather than the general "ignoring Unicode case", if possible without ruining performance. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:199 > +#if ENABLE(PLATFORM_FONT_LOOKUP) Should have a blank line after this to match the other formatting. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:200 > + if (family.startsWith(".")) More efficient to just check the first character rather than calling this helper. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:202 > + const HashSet<String>& whitelist = fontWhitelist(); Should use auto& for the type here. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210 > + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size); Looks like this CTFont will leak. Need to write something more like this: font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)); But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this: font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease]; > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:224 > NSFontManager *fontManager = [NSFontManager sharedFontManager]; > NSString *availableFamily = cachedAvailableFamily; Need to remove these two lines of code. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:296 > +#endif Should have a blank line before this to match the other formatting. > Source/WebKit/mac/WebView/WebView.mm:8820 > + HashSet<String> wtfWhitelist; I’d like to see: Vector<String> vector; > Source/WebKit/mac/WebView/WebView.mm:8821 > + for (NSString *nsString in whitelist) Should just name this "string". > Source/WebKit/mac/WebView/WebViewPrivate.h:1043 > ++ (void)_setFontWhitelist:(NSSet *)whitelist; This should just take an NSArray of NSString, not an NSSet. It’s not helpful to have this be a set when it’s case insensitive strings anyway; the set won’t detect enough duplicates to be the correct data structure. > Source/WebKit2/Shared/WebProcessCreationParameters.h:114 > + HashSet<String> fontWhitelist; Vector<String> > Source/WebKit2/UIProcess/WebProcessPool.h:473 > + HashSet<String> m_fontWhitelist; Vector<String> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:480 > +static NSSet *getFontWhitelist() Should not have get in its name. > Tools/DumpRenderTree/mac/DumpRenderTree.mm:487 > + NSMutableSet *availableFontList = [[NSMutableSet alloc] initWithCapacity:[allowedFamilies count] * 2]; Why bother with this capacity estimate? > Tools/WebKitTestRunner/mac/TestControllerMac.mm:262 > +static WKArrayRef generateWhitelist() Should return a smart pointer, not a raw pointer. > Tools/WebKitTestRunner/mac/TestControllerMac.mm:266 > + NSArray* fontsForFamily = [[NSFontManager sharedFontManager] availableMembersOfFontFamily:fontFamily]; Incorrect formatting for NSArray * > Tools/WebKitTestRunner/mac/TestControllerMac.mm:268 > + for (NSArray* fontInfo in fontsForFamily) { Incorrect formatting for NSArray * > Tools/WebKitTestRunner/mac/TestControllerMac.mm:293 > + auto whitelist = adoptWK(generateWhitelist()); No need for this local variable; would read better in-line.
(In reply to comment #13) > CTFontCreateForCSS will never return a screen font. However, all our testing > assumes that ::only:: screen fonts are used. I am going to migrate all our > tests to using printer fonts. Once I do that, this patch can go in. Why is that change OK? Don’t we have screen fonts for a reason? Do we really want to remove the use of them? If so, why not make that change first, independently, before switching to CTFontCreateForCSS?
Wow, I accidentally marked this as r?. It is definitely NOT ready. Sorry for wasting your time, Darin. Regarding screen fonts, I did a considerable amount of work removing our use of them already (starting during winter break last year). We currently now never use screen fonts (which is good). This ChangeLog is from before that work; I haven't updated it.
(In reply to comment #16) > (In reply to comment #13) > > CTFontCreateForCSS will never return a screen font. However, all our testing > > assumes that ::only:: screen fonts are used. I am going to migrate all our > > tests to using printer fonts. Once I do that, this patch can go in. > > Why is that change OK? Don’t we have screen fonts for a reason? We removed support for screen fonts in http://trac.webkit.org/changeset/179368.
(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #13) > > > CTFontCreateForCSS will never return a screen font. However, all our testing > > > assumes that ::only:: screen fonts are used. I am going to migrate all our > > > tests to using printer fonts. Once I do that, this patch can go in. > > > > Why is that change OK? Don’t we have screen fonts for a reason? > > We removed support for screen fonts in > http://trac.webkit.org/changeset/179368. Can we fix the tests to always use printer fonts, then, as a separate step before landing this patch? Or is there some misunderstanding here.
Looks like there is a misunderstanding. Screen fonts should never be used anywhere. I believe this is currently the case.
Comment on attachment 250546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250546&action=review >> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210 >> + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size); > > Looks like this CTFont will leak. Need to write something more like this: > > font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)); > > But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this: > > font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease]; desiredWeight needs to be multiplied by 100 due to API contract
Comment on attachment 250546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250546&action=review >>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210 >>> + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size); >> >> Looks like this CTFont will leak. Need to write something more like this: >> >> font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)); >> >> But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this: >> >> font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease]; > > desiredWeight needs to be multiplied by 100 due to API contract We need to not run toAppKitFontWeight() in this case.
Created attachment 250738 [details] Patch
Comment on attachment 250738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250738&action=review > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:241 > + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size); Darin's comment about leaking
Created attachment 250772 [details] Patch
Comment on attachment 250772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250772&action=review > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243 > + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size); Yes, as you said, this still contains a leak. > Source/WebKit/mac/WebView/WebView.mm:8823 > + WebCore::FontCache::setFontWhitelist(vector); iOS compilation error: WebKit/mac/WebView/WebView.mm:8822:25: error: no type named 'setFontWhitelist' in 'WebCore::FontCache' > Source/WebKit/mac/WebView/WebViewPrivate.h:1044 > +@interface WebView (WebViewFontSelection) > ++ (void)_setFontWhitelist:(NSArray *)whitelist; > +@end The underlying feature is Mac-only, so maybe you want this to be Mac-only? Or maybe just let it do nothing on iOS?
Comment on attachment 250772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250772&action=review >> Source/WebKit/mac/WebView/WebViewPrivate.h:1044 >> +@end > > The underlying feature is Mac-only, so maybe you want this to be Mac-only? Or maybe just let it do nothing on iOS? I intend for it to just do nothing on iOS
Comment on attachment 250546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250546&action=review >>>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210 >>>> + font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size); >>> >>> Looks like this CTFont will leak. Need to write something more like this: >>> >>> font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)); >>> >>> But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this: >>> >>> font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease]; >> >> desiredWeight needs to be multiplied by 100 due to API contract > > We need to not run toAppKitFontWeight() in this case. I feel like the name of the function should have been named CFBridgingAutorelease(), since by inspection it appears that the object gets released before it gets retained :( Anyway, done.
Created attachment 250891 [details] Patch for committing
Comment on attachment 250891 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=250891&action=review > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243 > + font = (NSFont*)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size)) retain] autorelease]; There is no need for the retain and autorelease here nor for the cast to NSFont* (which is formatted incorrectly, missing the space before the *). Look at all the other uses of CFBridgingRelease in WebCore and you will see the pattern. Should just be: font = CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size));
Comment on attachment 250891 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=250891&action=review >> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243 >> + font = (NSFont*)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size)) retain] autorelease]; > > There is no need for the retain and autorelease here nor for the cast to NSFont* (which is formatted incorrectly, missing the space before the *). Look at all the other uses of CFBridgingRelease in WebCore and you will see the pattern. Should just be: > > font = CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size)); That makes more sense!
Created attachment 250930 [details] Patch for landing
Created attachment 251915 [details] Patch for landing
Committed r183562: <http://trac.webkit.org/changeset/183562>