Expose more font weights for -apple-system rdar://problem/20809399
Created attachment 252515 [details] Patch
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.
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?
Committed 183895. https://trac.webkit.org/changeset/183895
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.
Re-opened since this is blocked by bug 144786
Rollout fixed the flakiness.
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.
Created attachment 253007 [details] Patch
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.
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
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.
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
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.
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?
Created attachment 253078 [details] Patch
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
Created attachment 253085 [details] Patch
Created attachment 253091 [details] Patch
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.
Created attachment 253129 [details] Patch
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?
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.
Committed r184353: <http://trac.webkit.org/changeset/184353>
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.
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.
See https://bugs.webkit.org/show_bug.cgi?id=145146