Bug 143395 - [Cocoa] System fonts do not get correct tracking
Summary: [Cocoa] System fonts do not get correct tracking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-03 19:01 PDT by Myles C. Maxfield
Modified: 2015-04-07 18:09 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2015-04-03 19:03 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2015-04-03 21:03 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (568.89 KB, application/zip)
2015-04-03 21:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (611.06 KB, application/zip)
2015-04-03 21:51 PDT, Build Bot
no flags Details
WIP (10.25 KB, patch)
2015-04-03 22:01 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2015-04-06 15:32 PDT, Myles C. Maxfield
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (11.89 KB, patch)
2015-04-07 14:41 PDT, Myles C. Maxfield
mmaxfield: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-04-03 19:01:58 PDT
Cache whether or not a font is a system font
Comment 1 Myles C. Maxfield 2015-04-03 19:03:25 PDT
Created attachment 250115 [details]
Patch
Comment 2 Ryosuke Niwa 2015-04-03 19:08:12 PDT
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).
Comment 3 Myles C. Maxfield 2015-04-03 21:03:04 PDT
Created attachment 250119 [details]
Patch
Comment 4 Myles C. Maxfield 2015-04-03 21:04:07 PDT
<rdar://problem/19703237>
Comment 5 Build Bot 2015-04-03 21:47:52 PDT
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
Comment 6 Build Bot 2015-04-03 21:47:56 PDT
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
Comment 7 Build Bot 2015-04-03 21:50:57 PDT
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
Comment 8 Build Bot 2015-04-03 21:51:01 PDT
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
Comment 9 Myles C. Maxfield 2015-04-03 22:01:22 PDT
Created attachment 250123 [details]
WIP
Comment 10 Myles C. Maxfield 2015-04-06 15:32:56 PDT
Created attachment 250232 [details]
Patch
Comment 11 Ryosuke Niwa 2015-04-06 16:26:22 PDT
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?
Comment 12 Myles C. Maxfield 2015-04-06 16:30:23 PDT
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.
Comment 13 Myles C. Maxfield 2015-04-06 16:31:27 PDT
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!
Comment 14 Myles C. Maxfield 2015-04-06 16:38:03 PDT
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.
Comment 15 Ryosuke Niwa 2015-04-06 16:46:00 PDT
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.
Comment 16 Myles C. Maxfield 2015-04-06 17:50:56 PDT
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.
Comment 17 Myles C. Maxfield 2015-04-07 14:41:24 PDT
Created attachment 250305 [details]
Patch for landing
Comment 18 Myles C. Maxfield 2015-04-07 18:09:15 PDT
Committed r182512: <http://trac.webkit.org/changeset/182512>