Bug 223062 – [macOS] MobileAsset fonts are broken in Reader mode in Safari

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

MobileAsset fonts are broken in Reader mode in Safari

[Myles C. Maxfield]

Created attachment 422913 [details]
WIP

[Myles C. Maxfield]

Created attachment 422917 [details]
WIP

[Myles C. Maxfield]

Created attachment 423003 [details]
Needs test

[Myles C. Maxfield]

Created attachment 423005 [details]
Patch

[Myles C. Maxfield]

Created attachment 423007 [details]
Patch

[Myles C. Maxfield]

Created attachment 423016 [details]
Patch

[Myles C. Maxfield]

Created attachment 423017 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 423017 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:550
> -// Defaults to false.
> +// Defaults to true.

Is this a correction to the comment, or a indicative of a behavior change? Changelog needs to explain.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2242
> +- (void)_switchFromStaticFontRegistryToUserFontRegistry

Since readers probably won't understand the implications of static vs. user font registries, a comment would help.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7868
>          SandboxExtension::Handle fontMachExtensionHandle;
>          SandboxExtension::createHandleForMachLookup("com.apple.fonts"_s, WTF::nullopt, fontMachExtensionHandle);

It's a bit odd to see this in two places? Share with switchFromStaticFontRegistryToUserFontRegistry()?

[Alex Christensen]

Comment on attachment 423017 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:453
> +- (void)_switchFromStaticFontRegistryToUserFontRegistry WK_API_AVAILABLE(macos(WK_MAC_TBA));

Is there a way to switch back?

[Myles C. Maxfield]

Comment on attachment 423017 [details]
Patch

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

>> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:550
>> +// Defaults to true.
> 
> Is this a correction to the comment, or a indicative of a behavior change? Changelog needs to explain.

A correction. I'll explain in the ChangeLog.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2242
>> +- (void)_switchFromStaticFontRegistryToUserFontRegistry
> 
> Since readers probably won't understand the implications of static vs. user font registries, a comment would help.

Good idea.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:453
>> +- (void)_switchFromStaticFontRegistryToUserFontRegistry WK_API_AVAILABLE(macos(WK_MAC_TBA));
> 
> Is there a way to switch back?

Nope!

[Myles C. Maxfield]

Created attachment 423068 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 423070 [details]
Patch for committing

[EWS]

Committed r274374: <https://commits.webkit.org/r274374>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423070 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75381965>

[Per Arne Vollan]

Comment on attachment 423017 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:7868
>>          SandboxExtension::createHandleForMachLookup("com.apple.fonts"_s, WTF::nullopt, fontMachExtensionHandle);
> 
> It's a bit odd to see this in two places? Share with switchFromStaticFontRegistryToUserFontRegistry()?

Should CTFontManagerEnableAllUserFonts(true) be called when this extension is consumed in WP?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1169
> +    m_fontMachExtension->consume();

To avoid storing the extension, you could also call SandboxExtension::consumePermanently here.

[Per Arne Vollan]

Comment on attachment 423017 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2242
>>> +- (void)_switchFromStaticFontRegistryToUserFontRegistry
>> 
>> Since readers probably won't understand the implications of static vs. user font registries, a comment would help.
> 
> Good idea.

Is this SPI strictly needed, since the client can use the preference shouldAllowUserInstalledFonts to enable this behavior?