[Cocoa] System fonts do not get correct tracking

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Cache whether or not a font is a system font

[Myles C. Maxfield]

Created attachment 250115 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 250115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250115&action=review

> Source/WebCore/platform/graphics/Font.h:308
> +    bool m_isSystemFont = { false };
> +

We should probabbly put this behinde PLATFORM(WIN) || PLATFORM(COCOA).

[Myles C. Maxfield]

Created attachment 250119 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/19703237>

[Build Bot]

Comment on attachment 250119 [details]
Patch

Attachment 250119 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6004488959688704

New failing tests:
fast/writing-mode/vertical-font-vmtx-units-per-em.html
fast/writing-mode/broken-ideograph-small-caps.html

[Build Bot]

Created attachment 250120 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250119 [details]
Patch

Attachment 250119 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4513084987146240

New failing tests:
fast/writing-mode/vertical-font-vmtx-units-per-em.html
fast/writing-mode/broken-ideograph-small-caps.html

[Build Bot]

Created attachment 250121 [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

[Myles C. Maxfield]

Created attachment 250123 [details]
WIP

[Myles C. Maxfield]

Created attachment 250232 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 250232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250232&action=review

> Source/WebCore/ChangeLog:30
> +        (WebCore::Font::platformInit): Cache whether or not the font is a system
> +        font.

Maybe we can fit "font." in the previous line?  It looks odd.

> Source/WebCore/platform/graphics/Font.cpp:60
> +    , m_mathData(nullptr)
> +#if ENABLE(OPENTYPE_VERTICAL)
> +    , m_verticalData(0)
> +#endif

m_verticalData doesn't need to be initialized since it's a RefPtr.
Why don't we initialize other values in the class declartion as in:
mutable RefPtr<OpenTypeMathData> m_mathData { nullptr };
so that we don't duplicate so much code in each constructor.
We can also add a private Font::Font to initialize commonly initialized member variables and just call that here.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:508
> +    } else if (!populatedAdvance) {
> +        if (isSystemFont())
> +            CTFontGetAdvancesForGlyphs(m_platformData.font(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
> +        else
> +            CTFontGetAdvancesForGlyphs(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
> +    }

Could you explain what this change entails in the change log?

[Myles C. Maxfield]

Comment on attachment 250232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250232&action=review

>> Source/WebCore/platform/graphics/Font.cpp:60
>> +#endif
> 
> m_verticalData doesn't need to be initialized since it's a RefPtr.
> Why don't we initialize other values in the class declartion as in:
> mutable RefPtr<OpenTypeMathData> m_mathData { nullptr };
> so that we don't duplicate so much code in each constructor.
> We can also add a private Font::Font to initialize commonly initialized member variables and just call that here.

Good idea.

[Myles C. Maxfield]

Comment on attachment 250232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250232&action=review

>>> Source/WebCore/platform/graphics/Font.cpp:60
>>> +#endif
>> 
>> m_verticalData doesn't need to be initialized since it's a RefPtr.
>> Why don't we initialize other values in the class declartion as in:
>> mutable RefPtr<OpenTypeMathData> m_mathData { nullptr };
>> so that we don't duplicate so much code in each constructor.
>> We can also add a private Font::Font to initialize commonly initialized member variables and just call that here.
> 
> Good idea.

Interestingly enough, the second constructor is SVG-specific and will be going away as soon!

[Myles C. Maxfield]

Comment on attachment 250232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250232&action=review

>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:508
>> +    }
> 
> Could you explain what this change entails in the change log?

It's quite straightforward - ctFont() is the round-tripped font, while font() is the original. advanced tracking stuff doesn't survive the round-trip, so we switch on it. We have some tests which rely on this being the round-tripped font on other cases. I'll add that to the ChangeLog.

[Ryosuke Niwa]

Comment on attachment 250232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250232&action=review

>>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:508
>>> +    }
>> 
>> Could you explain what this change entails in the change log?
> 
> It's quite straightforward - ctFont() is the round-tripped font, while font() is the original. advanced tracking stuff doesn't survive the round-trip, so we switch on it. We have some tests which rely on this being the round-tripped font on other cases. I'll add that to the ChangeLog.

Okay, r=me with that comment in the change log or perhaps even in the code.

[Myles C. Maxfield]

Darin gave me some really good advice on this bug. His advice has pretty large scope, though, so I think I'll commit this (with rniwa's comments) and address Darin's comments in a follow-up patch.

[Myles C. Maxfield]

Created attachment 250305 [details]
Patch for landing

[Myles C. Maxfield]

Committed r182512: <http://trac.webkit.org/changeset/182512>