Bug 221103 – font-stretch is not applied to system-ui

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

[Myles C. Maxfield]

font-stretch is not applied to system-ui

[Myles C. Maxfield]

Created attachment 418668 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/73719139>

[Myles C. Maxfield]

Created attachment 418669 [details]
Patch

[Darin Adler]

Comment on attachment 418668 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:855
> +#else
> +#define HAVE_LEVEL_1_SYSTEM_FONT_WIDTH_VALUES 1

This doesn’t seem right. It’s going to be set for non-Cocoa platforms? Maybe it should be #elif PLATFORM(COCOA)? Or omitted entirely. It seems we don’t use it.

> Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:46
> +         FilePath = "/Users/mmaxfield/Build/Products/Debug/MiniBrowser.app">

Probably should not land this. Irritating that this is so easy to do by accident — I find this kind of thing in my patches regularly too.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:84
> +    // FIXME: Use createFontByApplyingWeightWidthItalicsAndFallbackBehavior() once <rdar://problem/33046041> is fixed.

The wording of this comment seems outdated. That bug was fixed in 2017.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:227
> +    for (size_t i = 0; i < WTF_ARRAY_LENGTH(piecewisePoints) - 1; ++i) {

Can use std::size instead of WTF_ARRAY_LENGTH.

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:234
> +    return piecewisePoints[WTF_ARRAY_LENGTH(piecewisePoints) - 1].output;

Ditto.

> LayoutTests/ChangeLog:9
> +        * fast/text/system-font-width-expected-mismatch.html: Added.

With multiple tables of different weights and quite a bit of logic, seems unlikely that a single test covers enough to detect most mistakes. Further, a mismatch test is so susceptible to false negatives. I wish for a better testing strategy, without knowing what it would be.

[Darin Adler]

Comment on attachment 418669 [details]
Patch

My comments were on the previous version of the patch. I think they all still apply.

[Myles C. Maxfield]

Created attachment 418684 [details]
Patch for committing

[Darin Adler]

Comment on attachment 418684 [details]
Patch for committing

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

> Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:47
> +      <PathRunnable
> +         runnableDebuggingMode = "0"
> +         BundleIdentifier = "org.webkit.MiniBrowser"
> +         FilePath = "/Users/mmaxfield/Build/Products/Debug/MiniBrowser.app">
> +      </PathRunnable>

This is still in the patch!

[Myles C. Maxfield]

Created attachment 418691 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 418701 [details]
Patch for committing

[Myles C. Maxfield]

Committed r272073: <https://trac.webkit.org/changeset/272073>