[Cocoa] Reduce uses of CGFonts
Created attachment 286170 [details] WIP
Created attachment 286171 [details] WIP
Comment on attachment 286171 [details] WIP Attachment 286171 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1882508 Number of test failures exceeded the failure limit.
Created attachment 286187 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 286171 [details] WIP Attachment 286171 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1882562 Number of test failures exceeded the failure limit.
Created attachment 286191 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286193 [details] WIP
Created attachment 286224 [details] WIP
Created attachment 286237 [details] Patch
Comment on attachment 286237 [details] Patch Attachment 286237 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1884168 New failing tests: fast/attachment/attachment-icon-from-file-extension.html
Created attachment 286242 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286237 [details] Patch Attachment 286237 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1884171 New failing tests: fast/attachment/attachment-icon-from-file-extension.html
Created attachment 286243 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286237 [details] Patch Attachment 286237 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1884155 Number of test failures exceeded the failure limit.
Created attachment 286245 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 286295 [details] WIP
Created attachment 286394 [details] WIP
Created attachment 286400 [details] WIP
Created attachment 286408 [details] Patch
Comment on attachment 286408 [details] Patch Attachment 286408 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1895384 Number of test failures exceeded the failure limit.
Created attachment 286416 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 286419 [details] WIP
Created attachment 286442 [details] Patch
Created attachment 286443 [details] WIP
Created attachment 286445 [details] Patch
Created attachment 286446 [details] Patch
Comment on attachment 286446 [details] Patch Attachment 286446 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1898022 New failing tests: fast/text/international/vertical-text-glyph-test.html fast/text/international/vertical-text-metrics-test.html
Created attachment 286448 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286446 [details] Patch Attachment 286446 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1898021 New failing tests: fast/text/international/vertical-text-glyph-test.html fast/text/international/vertical-text-metrics-test.html
Created attachment 286449 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 286446 [details] Patch Attachment 286446 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1898028 New failing tests: fast/text/international/vertical-text-glyph-test.html fast/text/international/vertical-text-metrics-test.html
Created attachment 286450 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286446 [details] Patch Attachment 286446 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1898087 New failing tests: svg/custom/glyph-transformation-with-hkern.svg
Created attachment 286451 [details] Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 286462 [details] WIP
Created attachment 286464 [details] Patch
Comment on attachment 286464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286464&action=review > Source/WebCore/platform/graphics/FontMetrics.h:35 > FontMetrics() > - : m_unitsPerEm(gDefaultUnitsPerEm) > - , m_ascent(0) > - , m_descent(0) > - , m_lineGap(0) > - , m_lineSpacing(0) > - , m_xHeight(0) > - , m_zeroWidth(0) > - , m_hasXHeight(false) > - , m_hasZeroWidth(false) > { > } We should just omit this constructor entirely. The compiler will generate it without us defining it. > Source/WebCore/platform/graphics/FontMetrics.h:100 > - int capHeight() const { return lroundf(m_capHeight); } > + int capHeight() const { return lroundf(m_capHeight.value()); } This is a change in behavior. We will now assert if this is called when we don’t have a cap height; before we would presumably return 0. Should this return valueOr(0)? > Source/WebCore/platform/graphics/FontMetrics.h:125 > + unsigned m_unitsPerEm { gDefaultUnitsPerEm }; Naming here with "g" is so peculiar. > Source/WebCore/platform/graphics/FontMetrics.h:132 > + Optional<float> m_xHeight; > + Optional<float> m_capHeight; This does not seem right. The code in this class seems to handle Nullopt and 0 identically for both of these. I can’t find any code that handled a missing height differently from 0. Although some of the new code will now abort inside Optional in that case. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:92 > + RetainPtr<NSString> familyName = adoptNS((NSString *)CTFontCopyFamilyName(font)); The reason we have separate adoptNS and adoptCF functions was originally that under garbage collection the two operations are different. It’s generally not OK to just cast a CF pointer to NS and then do the adopt in the NS context. At the moment it might be OK since WebKit no longer supports garbage collection, but later when adopt ARC it will again likely be important to not use adoptNS on a CF Copy/Create function result. So this needs to use adoptCF instead of adoptNS. We can still cast the result to NSString if we want to use NS methods on it, but not in the RetainPtr. We could also just use CFStringCompare or put in a WTF::String and use equalIgnoringASCIICase functions. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:94 > + if (![familyName length]) > + return false; I don’t think it’s important to optimize the empty string case. I am guessing this is because some other code path was going to use CFStringCompare and in that case we need a null check. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:101 > + if ([familyName.get() compare:@"Times" options:NSCaseInsensitiveSearch] == NSOrderedSame > + || [familyName.get() compare:@"Helvetica" options:NSCaseInsensitiveSearch] == NSOrderedSame > + || [familyName.get() compare:@".Helvetica NeueUI" options:NSCaseInsensitiveSearch] == NSOrderedSame) > + return true; > + > + return false; Typically instead of if (x) return true; return false; we just return x; > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:144 > + RetainPtr<CFStringRef> familyName = adoptCF(CTFontCopyFamilyName(m_platformData.font())); I suggest using auto here for the RetainPtr type. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:145 > +#if !PLATFORM(IOS) I think this is clearer as #if PLATFORM(MAC). > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:154 > ascent += floorf(((ascent + descent) * 0.15f) + 0.5f); Should use std::floor instead of floorf. Also seems like this is rounding, so I would think std::round would be better than std::floor(x + 0.5). Also seems that the "is this one of the three font family names that need adjustment" code should go into a helper function. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:176 > + float xHeight = CGFontGetDescriptor(m_platformData.cgFont(), &descriptor) ? (descriptor.xHeight / 1000) * CTFontGetSize(m_platformData.font()) : 0; Should consider using auto here instead of float. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:180 > + lineGap = ceilf(lineGap); > + lineSpacing = ceil(ascent) + adjustment + ceil(descent) + lineGap; > + ascent = ceilf(ascent + adjustment); > + descent = ceilf(descent); Should use std::ceil instead of ceilf/ceil, which end up converting back and forth between float and double. In general this code seems confused about float vs. double vs. CGFloat and this will lead to slowness as we convert things back and forth. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:212 > + RetainPtr<CFDataRef> os2Table = adoptCF(CTFontCopyTable(m_platformData.font(), kCTFontTableOS2, kCTFontTableOptionNoOptions)); auto > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:219 > + RetainPtr<CFDataRef> headTable = adoptCF(CTFontCopyTable(m_platformData.font(), kCTFontTableHead, kCTFontTableOptionNoOptions)); auto > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:566 > + int fixedPitch = 0; > + if (RetainPtr<CFNumberRef> fixedPitchNumber = adoptCF(static_cast<CFNumberRef>(CTFontCopyAttribute(m_platformData.font(), kCTFontFixedAdvanceAttribute)))) > + CFNumberGetValue(fixedPitchNumber.get(), kCFNumberIntType, &fixedPitch); I think auto is better here than RetainPtr<CFNumberRef>. But also, I think a helper function to get an int out of a possibly-null CFNumberRef would make this cleaner. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:567 > + m_treatAsFixedPitch = (CTFontGetSymbolicTraits(ctFont) & kCTFontMonoSpaceTrait) || fixedPitch || (caseInsensitiveCompare(fullName.get(), CFSTR("Osaka-Mono")) || caseInsensitiveCompare(fullName.get(), CFSTR("MS-PGothic")) || caseInsensitiveCompare(fullName.get(), CFSTR("MonotypeCorsiva"))); The caseInsensitiveCompare function used here should be used elsewhere in this file. I see lots of other places both above and below that do not use it. > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:169 > + RetainPtr<CFStringRef> fontDescription = adoptCF(CFCopyDescription(font())); Please use auto here. > Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:60 > + if (RetainPtr<CFDataRef> os2Table = adoptCF(CTFontCopyTable(font, kCTFontTableOS2, kCTFontTableOptionNoOptions))) { auto > Source/WebCore/platform/graphics/win/FontCGWin.cpp:176 > + CGAffineTransform savedMatrix = CGContextGetTextMatrix(cgContext); > CGContextSetTextMatrix(cgContext, matrix); Annoying to not have the ScopedTextMatrix to use here. Is there a header for CG within WebCore that we can use?
Created attachment 286680 [details] Patch for committing
Created attachment 286687 [details] Patch for committing
Comment on attachment 286687 [details] Patch for committing Attachment 286687 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1926331 Number of test failures exceeded the failure limit.
Created attachment 286690 [details] Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286687 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=286687&action=review > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:99 > + if (!familyName || CFStringGetLength(familyName.get())) Whoops.
Comment on attachment 286464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286464&action=review >> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:176 >> + float xHeight = CGFontGetDescriptor(m_platformData.cgFont(), &descriptor) ? (descriptor.xHeight / 1000) * CTFontGetSize(m_platformData.font()) : 0; > > Should consider using auto here instead of float. Unfortunately, tests start failing if you change the precision. I intend to fix this, but I will do it in a separate patch because this one is supposed to have no behavior change. >> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:180 >> + descent = ceilf(descent); > > Should use std::ceil instead of ceilf/ceil, which end up converting back and forth between float and double. > > In general this code seems confused about float vs. double vs. CGFloat and this will lead to slowness as we convert things back and forth. Ditto.
Created attachment 286763 [details] Patch for committing
Committed r204858: <http://trac.webkit.org/changeset/204858>