Make FontCache::m_shouldMockBoldSystemFontForAccessibility static
Created attachment 446480 [details] Patch
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?
(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.
(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.
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?
(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?
(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.
(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.
Created attachment 446674 [details] Patch propagating to all FontCaches
I really don't know that this is better than having a static variable that all FontCache instances can consult.
<rdar://problem/86553539>
*** Bug 234057 has been marked as a duplicate of this bug. ***