Bug 228584 - [macOS] Remove unneeded sandbox extension member
Summary: [macOS] Remove unneeded sandbox extension member
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
: 228583 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-07-29 08:27 PDT by Per Arne Vollan
Modified: 2021-08-02 10:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.31 KB, patch)
2021-07-29 08:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2021-07-29 11:02 PDT, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2021-08-02 09:59 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2021-07-29 08:27:12 PDT
The commit https://trac.webkit.org/changeset/272999/webkit disabled this, but after adopting API to switch from static font registry to user font registry, this can be re-enabled.
Comment 1 Per Arne Vollan 2021-07-29 08:30:14 PDT
Created attachment 434521 [details]
Patch
Comment 2 Sam Weinig 2021-07-29 09:52:03 PDT
Comment on attachment 434521 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1282
> +void WebProcess::switchFromStaticFontRegistryToUserFontRegistry(const WebKit::SandboxExtension::Handle& fontMachExtensionHandle)

What is the point of changing this to an l-value? In general, handles should always be passed by r-value.
Comment 3 Per Arne Vollan 2021-07-29 11:02:33 PDT
Created attachment 434537 [details]
Patch
Comment 4 Per Arne Vollan 2021-07-29 11:03:01 PDT
(In reply to Sam Weinig from comment #2)
> Comment on attachment 434521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434521&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1282
> > +void WebProcess::switchFromStaticFontRegistryToUserFontRegistry(const WebKit::SandboxExtension::Handle& fontMachExtensionHandle)
> 
> What is the point of changing this to an l-value? In general, handles should
> always be passed by r-value.

Ah, good point. I have reverted back to the previous signature.

Thanks for reviewing!
Comment 5 Myles C. Maxfield 2021-07-29 11:45:07 PDT
Does this actually improve performance on OSes which have a static registry?
Comment 6 Per Arne Vollan 2021-07-29 12:04:47 PDT
(In reply to Myles C. Maxfield from comment #5)
> Does this actually improve performance on OSes which have a static registry?

Good point, I will start testing this now.

Thanks for reviewing!
Comment 7 Darin Adler 2021-07-29 13:17:27 PDT
My review+ assumes that this works as expected and improves performance.
Comment 8 Per Arne Vollan 2021-07-29 13:30:42 PDT
(In reply to Darin Adler from comment #7)
> My review+ assumes that this works as expected and improves performance.

Will test performance before landing.

Thanks for reviewing!
Comment 9 Alexey Proskuryakov 2021-07-31 18:12:10 PDT
*** Bug 228583 has been marked as a duplicate of this bug. ***
Comment 10 Per Arne Vollan 2021-08-02 09:56:37 PDT
(In reply to Per Arne Vollan from comment #8)
> (In reply to Darin Adler from comment #7)
> > My review+ assumes that this works as expected and improves performance.
> 
> Will test performance before landing.
> 
> Thanks for reviewing!

Landing just the sandbox extension bits, since re-enabling the font pre-warming does not show a meaningful performance improvement.
Comment 11 Per Arne Vollan 2021-08-02 09:59:15 PDT
Created attachment 434765 [details]
Patch
Comment 12 EWS 2021-08-02 10:29:12 PDT
Committed r280542 (240169@main): <https://commits.webkit.org/240169@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434765 [details].
Comment 13 Radar WebKit Bug Importer 2021-08-02 10:30:18 PDT
<rdar://problem/81418101>