Bug 173043 – [Cocoa] Expand system-ui to include every item in the Core Text cascade list

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

[Myles C. Maxfield]

Make system-ui not suck

[Myles C. Maxfield]

Created attachment 312149 [details]
WIP

[Myles C. Maxfield]

Created attachment 312150 [details]
WIP

[Myles C. Maxfield]

Created attachment 312156 [details]
WIP

[Myles C. Maxfield]

Created attachment 312263 [details]
WIP

[Myles C. Maxfield]

Created attachment 312285 [details]
Patch

[Build Bot]

Comment on attachment 312285 [details]
Patch

Attachment 312285 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3895013

New failing tests:
fast/text/international/pop-up-button-text-alignment-and-direction.html
fast/forms/visual-hebrew-text-field.html
fast/forms/searchfield-heights.html
fast/forms/listbox-bidi-align.html
fast/forms/select-visual-hebrew.html
fast/text/system-font-fallback-emoji.html
fast/css/rtl-ordering.html
fast/text/updateNewFont.html
fast/text/drawBidiText.html
fast/forms/search-rtl.html
fast/forms/listbox-hit-test-zoomed.html

[Build Bot]

Created attachment 312325 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 312285 [details]
Patch

Attachment 312285 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3895051

New failing tests:
fast/text/international/pop-up-button-text-alignment-and-direction.html
fast/forms/visual-hebrew-text-field.html
fast/forms/searchfield-heights.html
fast/forms/listbox-bidi-align.html
fast/forms/select-visual-hebrew.html
fast/text/system-font-fallback-emoji.html
fast/css/rtl-ordering.html
fast/text/updateNewFont.html
fast/text/drawBidiText.html
fast/forms/listbox-hit-test-zoomed.html
fast/forms/search-rtl.html

[Build Bot]

Created attachment 312327 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Created attachment 312332 [details]
Patch

[Build Bot]

Comment on attachment 312332 [details]
Patch

Attachment 312332 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3895570

New failing tests:
fast/mediastream/getUserMedia-grant-persistency.html

[Build Bot]

Created attachment 312336 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Looks like failure is unrelated.

[Myles C. Maxfield]

<rdar://problem/31639090>

[Myles C. Maxfield]

<rdar://problem/21125708>

[Myles C. Maxfield]

Comment on attachment 312332 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=173043

<rdar://problem/21125708>

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=173043

<rdar://problem/21125708>

[Myles C. Maxfield]

Created attachment 312354 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 312354 [details]
Patch

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

> Source/WebCore/platform/graphics/FontDescription.h:41
> +#include "FontFamilySpecificationCoreText.h"
> +#else
> +#include "FontFamilyPlatformSpecification.h"

The name mismatch here is a bit jarring.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:732
> +    CFNotificationCenterAddObserver(center, this, &fontCacheRegisteredFontsChangedNotificationCallback, notificationName, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately);

Do we need to call CFNotificationCenterRemove{Every}Observer at some point?

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:85
> +    Vector<RetainPtr<CTFontDescriptorRef>> systemFontCascadeList(CoreTextCascadeListParameters parameters)

const CoreTextCascadeListParameters&

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:105
> +    RetainPtr<CTFontRef> applyWeightAndItalics(CTFontRef font, CGFloat weight, bool italic, float size)

Can this be const or static? I don't see it changing local state.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:121
> +    Vector<RetainPtr<CTFontDescriptorRef>> computeCascadeList(CTFontRef font, CFStringRef locale)

Can this be const or static? I don't see it changing local state.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:182
> +    auto weight = description.weight();
> +    if (weight < FontSelectionValue(150))
> +        result.weight = kCTFontWeightUltraLight;
> +    else if (weight < FontSelectionValue(250))
> +        result.weight = kCTFontWeightThin;
> +    else if (weight < FontSelectionValue(350))
> +        result.weight = kCTFontWeightLight;
> +    else if (weight < FontSelectionValue(450))
> +        result.weight = kCTFontWeightRegular;
> +    else if (weight < FontSelectionValue(550))
> +        result.weight = kCTFontWeightMedium;
> +    else if (weight < FontSelectionValue(650))
> +        result.weight = kCTFontWeightSemibold;
> +    else if (weight < FontSelectionValue(750))
> +        result.weight = kCTFontWeightBold;
> +    else if (weight < FontSelectionValue(850))
> +        result.weight = kCTFontWeightHeavy;
> +    else
> +        result.weight = kCTFontWeightBlack;

This feels like the reverse mapping of code I reviewed a short time ago. Would be nice to put both in the the same place.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:214
> +            index -= cascadeList.size();

What guarantees that index doesn't underflow?

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:218
> +            --index;

Ditto.

[Myles C. Maxfield]

Comment on attachment 312354 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontDescription.h:41
>> +#include "FontFamilyPlatformSpecification.h"
> 
> The name mismatch here is a bit jarring.

FontFamilyPlatformSpecification.h => FontFamilySpecificationNull.h

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:732
>> +    CFNotificationCenterAddObserver(center, this, &fontCacheRegisteredFontsChangedNotificationCallback, notificationName, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately);
> 
> Do we need to call CFNotificationCenterRemove{Every}Observer at some point?

FontCache is a singleton that never gets destroyed, so we don't.

>> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:214
>> +            index -= cascadeList.size();
> 
> What guarantees that index doesn't underflow?

Line 212, if (index < cascadeList.size())

>> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:218
>> +            --index;
> 
> Ditto.

Line 215, if (!index)

Comment on attachment 312354 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:995
> +		1C12AC2B1EE778AE0079E0A0 /* FontFamilySpecificationCoreText.h in Headers */ = {isa = PBXBuildFile; fileRef = 1C12AC291EE778AE0079E0A0 /* FontFamilySpecificationCoreText.h */; settings = {ATTRIBUTES = (Private, ); }; };

Does this really need to be a private header?

[Myles C. Maxfield]

Comment on attachment 312354 [details]
Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:995
>> +		1C12AC2B1EE778AE0079E0A0 /* FontFamilySpecificationCoreText.h in Headers */ = {isa = PBXBuildFile; fileRef = 1C12AC291EE778AE0079E0A0 /* FontFamilySpecificationCoreText.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Does this really need to be a private header?

Yes, because it's included from FontDescription.h which is also a private header. WebKit doesn't compile if it isn't.

[Myles C. Maxfield]

Committed r217955: <http://trac.webkit.org/changeset/217955>

[Matt Lewis]

It looks like this revision may have cause several API Failures on macOS Debug with:
WKWebView.NavigatorLanguage
WebKit1.NavigatorLanguage


It looks to be and assertion failure:
ASSERTION FAILED: CFEqual(name, kCTFontManagerRegisteredFontsChangedNotification)


https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/1607
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/1607/steps/run-api-tests/logs/stdio

[Alexey Proskuryakov]

This change was rolled back in r218009.

[Myles C. Maxfield]

WebPopupMenu::setUpPlatformData() is trying to encode a new font for IPC. This new font has a cascade list full of CTFontDescriptorRefs, and our IPC code doesn’t know how to handle those.

I could either teach our IPC code how to serialize a CTFontDescriptorRef, or I could just remove the kCTFontCascadeListAttribute key from the attributes dictionary.

[Myles C. Maxfield]

Created attachment 312592 [details]
Patch

[Myles C. Maxfield]

Created attachment 312722 [details]
Updated patch for committing

[Myles C. Maxfield]

Created attachment 312730 [details]
Updated patch for committing

[WebKit Commit Bot]

Comment on attachment 312730 [details]
Updated patch for committing

Clearing flags on attachment: 312730

Committed r218161: <http://trac.webkit.org/changeset/218161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

So the new patch seems to have fixed the API issue on the Mac Release builds but it is still failing on the Debug builds.

Debug builds:
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/1519
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/1519/steps/run-api-tests/logs/stdio

Release:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/2205

[Myles C. Maxfield]

(In reply to Matt Lewis from comment #31)
> So the new patch seems to have fixed the API issue on the Mac Release builds
> but it is still failing on the Debug builds.
> 
> Debug builds:
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/
> builds/1519
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/
> builds/1519/steps/run-api-tests/logs/stdio
> 
> Release:
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/2205

Yeah, I just forgot to update the ASSERT(). I'll do it ASAP.

Thanks for catching this.

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 312820 [details]
Test fix

[Matt Lewis]

*** Bug 173379 has been marked as a duplicate of this bug. ***

[Matt Lewis]

The Revision caused a Layout test failure for: fast/text/system-font-fallback-emoji.html

[Matt Lewis]

Reverted r218161 for reason:

Introduced bot API test failures and Layout Test Failures.

Committed r218287: <http://trac.webkit.org/changeset/218287>

[Myles C. Maxfield]

Created attachment 313004 [details]
Patch

[Myles C. Maxfield]

Created attachment 313026 [details]
Patch

[Myles C. Maxfield]

Created attachment 313031 [details]
Patch for committing

[Myles C. Maxfield]

Committed r218371: <http://trac.webkit.org/changeset/218371>

[Myles C. Maxfield]

Committed r218372: <http://trac.webkit.org/changeset/218372>