Bug 234058 – Make FontCache::m_shouldMockBoldSystemFontForAccessibility static

• Status: NEW
• Component: Layout and Rendering

[Cameron McCormack (:heycam)]

Make FontCache::m_shouldMockBoldSystemFontForAccessibility static

[Cameron McCormack (:heycam)]

Created attachment 446480 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 446480 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:151
> +    static bool shouldMockBoldSystemFontForAccessibility() { return s_shouldMockBoldSystemFontForAccessibility; }

don't you need to do a .load()?

> Source/WebCore/platform/graphics/FontCache.h:199
> +    static inline std::atomic<bool> s_shouldMockBoldSystemFontForAccessibility { false };

I think adding globals is kind of the wrong direction. There will already be multiple font caches; why can't a worker just read the (non-atomic) bool from its thread's font cache?

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 446480 [details]
> > Source/WebCore/platform/graphics/FontCache.h:151
> > +    static bool shouldMockBoldSystemFontForAccessibility() { return s_shouldMockBoldSystemFontForAccessibility; }
> 
> don't you need to do a .load()?

std::atomic<T> has an operator T.

> > Source/WebCore/platform/graphics/FontCache.h:199
> > +    static inline std::atomic<bool> s_shouldMockBoldSystemFontForAccessibility { false };
> 
> I think adding globals is kind of the wrong direction. There will already be
> multiple font caches; why can't a worker just read the (non-atomic) bool
> from its thread's font cache?

We can easily do that with the callOnAllFontCaches() function from bug 234060.  But it seems overkill for a field that when set doesn't need any other special processing to happen.

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> We can easily do that with the callOnAllFontCaches() function from bug
> 234060.  But it seems overkill for a field that when set doesn't need any
> other special processing to happen.

But I'm happy to change to use that if you prefer.

[Myles C. Maxfield]

Comment on attachment 446480 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontCache.h:199
>>> +    static inline std::atomic<bool> s_shouldMockBoldSystemFontForAccessibility { false };
>> 
>> I think adding globals is kind of the wrong direction. There will already be multiple font caches; why can't a worker just read the (non-atomic) bool from its thread's font cache?
> 
> We can easily do that with the callOnAllFontCaches() function from bug 234060.  But it seems overkill for a field that when set doesn't need any other special processing to happen.

I think that would be better.

I'm also not sure if callOnAllFontCaches() is needed. Do you think that's better than just setting the current thread's value?

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #5)
> I think that would be better.
> 
> I'm also not sure if callOnAllFontCaches() is needed. Do you think that's
> better than just setting the current thread's value?

So you're saying: existing tests only need this value to be set on the main thread, and so we shouldn't bother propagating it to workers?

[Myles C. Maxfield]

(In reply to Cameron McCormack (:heycam) from comment #6)
> (In reply to Myles C. Maxfield from comment #5)
> > I'm also not sure if callOnAllFontCaches() is needed. Do you think that's
> > better than just setting the current thread's value?
> 
> So you're saying: existing tests only need this value to be set on the main
> thread,

Yes.

> and so we shouldn't bother propagating it to workers?

I wasn't going that far as to make a statement. I was just asking the question. If you think it should be set on all the threads because future tests may want that functionality, then sure, that makes sense. But I could also imagine a worker test that require the worker to call this function itself. I think it's up to you.

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #7)
> I wasn't going that far as to make a statement. I was just asking the
> question. If you think it should be set on all the threads because future
> tests may want that functionality, then sure, that makes sense. But I could
> also imagine a worker test that require the worker to call this function
> itself. I think it's up to you.

OK that's a fair point.  Since this is set through Internals.settings, which feels like a global thing rather than a per-thread thing, I'm going to propagate to workers.

[Cameron McCormack (:heycam)]

Created attachment 446674 [details]
Patch propagating to all FontCaches

[Cameron McCormack (:heycam)]

I really don't know that this is better than having a static variable that all FontCache instances can consult.

[Radar WebKit Bug Importer]

<rdar://problem/86553539>

[Ahmad Saleem]

*** Bug 234057 has been marked as a duplicate of this bug. ***