Bug 233978

Summary: Make FontCascade::setShouldUseSmoothing/shouldUseSmoothing thread safe
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: Layout and RenderingAssignee: Matt Woodrow <mattwoodrow>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, mattwoodrow, mmaxfield, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233488    
Attachments:
Description Flags
Patch none

Description Cameron McCormack (:heycam) 2021-12-07 22:22:52 PST
Make FontCascade::setShouldUseSmoothing/shouldUseSmoothing thread safe
Comment 1 Cameron McCormack (:heycam) 2021-12-07 22:29:12 PST
Created attachment 446301 [details]
Patch
Comment 2 Myles C. Maxfield 2021-12-08 21:08:32 PST
Comment on attachment 446301 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        same time from the main thread. This setting is only changed from
> +        Internals and WebView and at WebProcess initilization, but should

WebKitLegacy and WebKit have SPI for this:

+ (void)_setShouldUseFontSmoothing:(BOOL)f
{
    WebCore::FontCascade::setShouldUseSmoothing(f);
}

void WKContextSetShouldUseFontSmoothing(WKContextRef contextRef, bool useFontSmoothing)
{
    WebKit::toImpl(contextRef)->setShouldUseFontSmoothing(useFontSmoothing);
}

> Source/WebCore/platform/graphics/FontCascade.cpp:442
> +static std::atomic<bool> shouldUseFontSmoothing = true;

Is it possible to make this no longer a global? I think that would be a better solution than this.
Comment 3 Radar WebKit Bug Importer 2021-12-14 22:23:15 PST
<rdar://problem/86506527>
Comment 4 Matt Woodrow 2022-09-07 19:24:45 PDT
(In reply to Myles C. Maxfield from comment #2)
> 
> > Source/WebCore/platform/graphics/FontCascade.cpp:442
> > +static std::atomic<bool> shouldUseFontSmoothing = true;
> 
> Is it possible to make this no longer a global? I think that would be a
> better solution than this.

We could store this on the per-thread FontCache instances, but that then makes it harder to mutate with the SPI, since we'd have to dispatch it to all threads that have a FontCache initialised.
Comment 5 Myles C. Maxfield 2022-09-07 19:46:00 PDT
(In reply to Matt Woodrow from comment #4)
> (In reply to Myles C. Maxfield from comment #2)
> > 
> > > Source/WebCore/platform/graphics/FontCascade.cpp:442
> > > +static std::atomic<bool> shouldUseFontSmoothing = true;
> > 
> > Is it possible to make this no longer a global? I think that would be a
> > better solution than this.
> 
> We could store this on the per-thread FontCache instances, but that then
> makes it harder to mutate with the SPI, since we'd have to dispatch it to
> all threads that have a FontCache initialised.

I guess I still think that's a better solution than a global :/
Comment 6 Myles C. Maxfield 2022-09-07 19:46:50 PDT
Alternatively, we could try to find all callers of the SPI and see if we can remove it.
Comment 7 Matt Woodrow 2022-09-11 15:32:44 PDT
Pull request: https://github.com/WebKit/WebKit/pull/4240
Comment 8 EWS 2022-09-14 17:38:25 PDT
Committed 254500@main (eb547e04a187): <https://commits.webkit.org/254500@main>

Reviewed commits have been landed. Closing PR #4240 and removing active labels.