Bug 223062 - [macOS] MobileAsset fonts are broken in Reader mode in Safari
Summary: [macOS] MobileAsset fonts are broken in Reader mode in Safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-11 01:11 PST by Myles C. Maxfield
Modified: 2021-03-16 08:48 PDT (History)
8 users (show)

See Also:


Attachments
WIP (4.58 KB, patch)
2021-03-11 01:11 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (12.77 KB, patch)
2021-03-11 02:14 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Needs test (12.28 KB, patch)
2021-03-11 18:24 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (24.08 KB, patch)
2021-03-11 19:17 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.02 KB, patch)
2021-03-11 19:32 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.75 KB, patch)
2021-03-11 23:01 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2021-03-11 23:05 PST, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for committing (27.44 KB, patch)
2021-03-12 11:23 PST, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for committing (27.27 KB, patch)
2021-03-12 11:48 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?