Bug 160896 – [Cocoa] Reduce uses of CGFonts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Reduce uses of CGFonts

[Myles C. Maxfield]

Created attachment 286170 [details]
WIP

[Myles C. Maxfield]

Created attachment 286171 [details]
WIP

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 286193 [details]
WIP

[Myles C. Maxfield]

Created attachment 286224 [details]
WIP

[Myles C. Maxfield]

Created attachment 286237 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 286295 [details]
WIP

[Myles C. Maxfield]

Created attachment 286394 [details]
WIP

[Myles C. Maxfield]

Created attachment 286400 [details]
WIP

[Myles C. Maxfield]

Created attachment 286408 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 286419 [details]
WIP

[Myles C. Maxfield]

Created attachment 286442 [details]
Patch

[Myles C. Maxfield]

Created attachment 286443 [details]
WIP

[Myles C. Maxfield]

Created attachment 286445 [details]
Patch

[Myles C. Maxfield]

Created attachment 286446 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 286462 [details]
WIP

[Myles C. Maxfield]

Created attachment 286464 [details]
Patch

[Darin Adler]

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?

[Myles C. Maxfield]

Created attachment 286680 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 286687 [details]
Patch for committing

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 286763 [details]
Patch for committing

[Myles C. Maxfield]

Committed r204858: <http://trac.webkit.org/changeset/204858>