Bug 223062

Summary: [macOS] MobileAsset fonts are broken in Reader mode in Safari
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, dino, jonlee, mmaxfield, pvollan, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
WIP
none
Needs test
none
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+
Patch for committing
ews-feeder: commit-queue-
Patch for committing none

Description Myles C. Maxfield 2021-03-11 01:11:00 PST 
MobileAsset fonts are broken in Reader mode in Safari
Comment 1 Myles C. Maxfield 2021-03-11 01:11:24 PST 
Created attachment 422913 [details]
WIP
Comment 2 Myles C. Maxfield 2021-03-11 02:14:36 PST 
Created attachment 422917 [details]
WIP
Comment 3 Myles C. Maxfield 2021-03-11 18:24:19 PST 
Created attachment 423003 [details]
Needs test
Comment 4 Myles C. Maxfield 2021-03-11 19:17:35 PST 
Created attachment 423005 [details]
Patch
Comment 5 Myles C. Maxfield 2021-03-11 19:32:04 PST 
Created attachment 423007 [details]
Patch
Comment 6 Myles C. Maxfield 2021-03-11 23:01:38 PST 
Created attachment 423016 [details]
Patch
Comment 7 Myles C. Maxfield 2021-03-11 23:05:31 PST 
Created attachment 423017 [details]
Patch
Comment 8 Simon Fraser (smfr) 2021-03-11 23:39:35 PST 
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()?
Comment 9 Alex Christensen 2021-03-12 07:53:24 PST 
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?
Comment 10 Myles C. Maxfield 2021-03-12 11:16:23 PST 
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!
Comment 11 Myles C. Maxfield 2021-03-12 11:23:01 PST 
Created attachment 423068 [details]
Patch for committing
Comment 12 Myles C. Maxfield 2021-03-12 11:48:16 PST 
Created attachment 423070 [details]
Patch for committing
Comment 13 EWS 2021-03-12 16:01:05 PST 
Committed r274374: <https://commits.webkit.org/r274374>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423070 [details].
Comment 14 Radar WebKit Bug Importer 2021-03-12 16:02:16 PST 
<rdar://problem/75381965>
Comment 15 Per Arne Vollan 2021-03-16 08:47:42 PDT 
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.
Comment 16 Per Arne Vollan 2021-03-16 08:48:53 PDT 
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?