Bug 144707 – [Mac] Expose more font weights for -apple-system

• Component: Text
• Status: RESOLVED FIXED

[Jon Lee]

Expose more font weights for -apple-system

rdar://problem/20809399

[Jon Lee]

Created attachment 252515 [details]
Patch

[Dean Jackson]

Comment on attachment 252515 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210
> +        NSFontWeightUltraLight, // Values between ultra-light and thin are swapped

Nit: Missing period at end of comment.

[Myles C. Maxfield]

Comment on attachment 252515 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:232
> +        return [NSFont systemFontOfSize:size weight:toNSFontWeight(weight)];

There's really not a good way to do this with CoreText?

[Jon Lee]

Committed 183895.
https://trac.webkit.org/changeset/183895

[Alexey Proskuryakov]

A number of tests became flaky after this commit. Ac couple examples: fast/text/international/unicode-bidi-plaintext-in-textarea.html and fast/text/updateNewFont.html

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Finternational%2Funicode-bidi-plaintext-in-textarea.html

I'm going to roll out to confirm that this patch was the culprit.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144786

[Alexey Proskuryakov]

Rollout fixed the flakiness.

[Jon Lee]

Figured out what the sequence was.

fast/css/multiple-tabs.html
fast/text/international/bidi-listbox-atsui.html
fast/text/international/bidi-listbox.html
fast/text/international/bidi-menulist.html
fast/text/international/hindi-spacing.html
fast/text/international/pop-up-button-text-alignment-and-direction.html
fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/text/updateNewFont.html

Running multiple-tabs.html causes the subsequent tests to fail.

[Myles C. Maxfield]

Created attachment 253007 [details]
Patch

[Build Bot]

Comment on attachment 253007 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 253007 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253011 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Darin Adler]

Comment on attachment 253007 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:211
> +        NSFontWeightUltraLight, // Values between ultra-light and thin are swapped.

I don’t understand the grammar in the comment. Are you saying that two weights are out of increasing order here?

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:229
> +    if (equalIgnoringASCIICase(family.string(), String("-webkit-system-font", String::ConstructFromLiteral))
> +        || equalIgnoringASCIICase(family.string(), String("-apple-system", String::ConstructFromLiteral))
> +        || equalIgnoringASCIICase(family.string(), String("-apple-system-font", String::ConstructFromLiteral))) {

This is not the way to compare ignoring ASCII case. We should overload. It’s OK to require ASCIILiteral because that’s the way to indicate that something is a literal, but it’s not good to construct and then destroy a String just to do a comparison with a constant.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:252
> +    return Optional<NSFont*>();

We use Nullopt for this.

> Source/WebCore/platform/mac/ThemeMac.mm:678
> +            fontDescription.setOneFamily(String("-apple-system", String::ConstructFromLiteral));

This should just be ASCIILiteral. Talk to Ben about the tradeoffs, please.

> Source/WebCore/rendering/RenderThemeMac.mm:359
> +            fontName = String("-apple-menu", String::ConstructFromLiteral);

ASCIILiteral please.

[Jon Lee]

Comment on attachment 253007 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        system font is, get that font's family name, and synthesize a font-family CSS propery for the element.

typo: property

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:211
>> +        NSFontWeightUltraLight, // Values between ultra-light and thin are swapped.
> 
> I don’t understand the grammar in the comment. Are you saying that two weights are out of increasing order here?

According to the W3C spec a font weight of Thin should be lighter than that of UltraLight, but AppKit's UltraLight is lighter than Thin. The weights as listed are in increasing order, even though semantically it appears two are swapped (I do not know which one is definitively correct).

The comment could be removed.

> Source/WebCore/rendering/RenderThemeMac.mm:382
> +        fontName = String("-apple-system", String::ConstructFromLiteral);

Why not just make this the default when you declare fontName?

[Myles C. Maxfield]

Created attachment 253078 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 253078 [details]
Patch

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

> LayoutTests/platform/mac-mavericks/fast/css/css2-system-fonts-expected.txt:2
> +caption: normal normal normal 13px/16px -apple-system

I might as well just delete this file

[Myles C. Maxfield]

Created attachment 253085 [details]
Patch

[Myles C. Maxfield]

Created attachment 253091 [details]
Patch

[Benjamin Poulain]

Comment on attachment 253091 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Create an overload for equalIgnoringASCIICase for string literals.

WTF LGTM.

> Source/WTF/wtf/text/WTFString.h:513
> +template<unsigned charactersCount>
> +inline bool equalIgnoringASCIICase(const String& a, const char (&b)[charactersCount]) { return equalIgnoringASCIICase(a.impl(), b); }

I would use
    equalIgnoringASCIICase<charactersCount>(a, b)
or
    equalIgnoringASCIICase(a, b, charactersCount - 1)

to enforce the right substitutions.

[Myles C. Maxfield]

Created attachment 253129 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 253129 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Previously, when we parse a CSS declaration of the form font: keyword; where keyword

parsed

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:221
> +    return nsFontWeights[fontWeight];

Should this assert that fontWeight is in range?

[Myles C. Maxfield]

Comment on attachment 253129 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:690
>      // System fonts are hidden by having a name that begins with a period, so simply search

This comment is no longer accurate.

[Myles C. Maxfield]

Committed r184353: <http://trac.webkit.org/changeset/184353>

[Darin Adler]

Comment on attachment 253129 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:690
>>      // System fonts are hidden by having a name that begins with a period, so simply search
> 
> This comment is no longer accurate.

I hope you fixed it!

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:227
> +    if (equalIgnoringASCIICase(family.string(), "-webkit-system-font")

I think this should be overloaded for AtomicString so we don’t have to do .string() explicitly.

> Source/WebCore/platform/mac/ThemeMac.mm:678
> +            fontDescription.setOneFamily(AtomicString("-apple-system", AtomicString::ConstructFromLiteral));

Please talk to Ben Poulain about this. I don’t think you should be using ConstructFromLiteral at call sites like this one. Maybe Ben can write down some guidelines so we can get this right.

> Source/WebCore/rendering/RenderThemeMac.mm:353
> +    String fontName(ASCIILiteral("-apple-system"));

Local variable should have type ASCIILiteral, not String, to avoid unnecessary extra memory allocation.

Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

> Source/WebCore/rendering/RenderThemeMac.mm:360
> +            fontName = String(ASCIILiteral("-apple-menu"));

Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

> Source/WebCore/rendering/RenderThemeMac.mm:364
> +            fontName = String(ASCIILiteral("-apple-status-bar"));

Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

> Source/WebCore/rendering/RenderThemeMac.mm:830
> +    fontDescription.setOneFamily(String("-apple-system", String::ConstructFromLiteral));

Please talk to Ben Poulain about this. I don’t think you should be using ConstructFromLiteral at call sites like this one. Maybe Ben can write down some guidelines so we can get this right.

Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

[Myles C. Maxfield]

Comment on attachment 253129 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:690
>>>      // System fonts are hidden by having a name that begins with a period, so simply search
>> 
>> This comment is no longer accurate.
> 
> I hope you fixed it!

I did before committing.

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:227
>> +    if (equalIgnoringASCIICase(family.string(), "-webkit-system-font")
> 
> I think this should be overloaded for AtomicString so we don’t have to do .string() explicitly.

Done.

>> Source/WebCore/platform/mac/ThemeMac.mm:678
>> +            fontDescription.setOneFamily(AtomicString("-apple-system", AtomicString::ConstructFromLiteral));
> 
> Please talk to Ben Poulain about this. I don’t think you should be using ConstructFromLiteral at call sites like this one. Maybe Ben can write down some guidelines so we can get this right.

I did; this is what Ben recommended.

>> Source/WebCore/rendering/RenderThemeMac.mm:353
>> +    String fontName(ASCIILiteral("-apple-system"));
> 
> Local variable should have type ASCIILiteral, not String, to avoid unnecessary extra memory allocation.
> 
> Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

Done.

>> Source/WebCore/rendering/RenderThemeMac.mm:360
>> +            fontName = String(ASCIILiteral("-apple-menu"));
> 
> Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

Done.

>> Source/WebCore/rendering/RenderThemeMac.mm:364
>> +            fontName = String(ASCIILiteral("-apple-status-bar"));
> 
> Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

Done.

>> Source/WebCore/rendering/RenderThemeMac.mm:830
>> +    fontDescription.setOneFamily(String("-apple-system", String::ConstructFromLiteral));
> 
> Please talk to Ben Poulain about this. I don’t think you should be using ConstructFromLiteral at call sites like this one. Maybe Ben can write down some guidelines so we can get this right.
> 
> Wasteful to create a String and then later turn that into an AtomicString. That means we will often be deleting the temporary StringImpl we created because we’ll find the already-existing AtomicStringImpl for this string.

I already fixed this in the version of the patch I committed.

[Myles C. Maxfield]

See https://bugs.webkit.org/show_bug.cgi?id=145146