FontCache currently maintains a secondary refcount for SimpleFontDatas. This is used to decide whether a font is considered inactive and is eligible for purging. This is confusing and complex.
Created attachment 244263 [details] patch
Attachment 244263 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/FontCache.cpp:434: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 244263 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=244263&action=review Great change. Build errors: ../../Source/WebCore/platform/graphics/FontCache.cpp: In member function 'void WebCore::FontCache::purgeInactiveFontData(int)': ../../Source/WebCore/platform/graphics/FontCache.cpp:459:47: error: 'gFontDataCache' was not declared in this scope ../../Source/WebCore/platform/graphics/FontCache.cpp:461:100: error: 'class WTF::RefPtr<WebCore::SimpleFontData>' has no member named 'first' > Source/WebCore/css/CSSFontFaceSource.cpp:147 > + RefPtr<SimpleFontData> placeholderFont = fontCache().lastResortFallbackFont(fontDescription); Why RefPtr instead of Ref? Also, normally I like to use auto for things like this. I’m kind of surprised that this function returns a smart pointer at all. Can it return a raw reference instead, or am I missing the point in some major way in asking that? > Source/WebCore/css/CSSFontFaceSource.cpp:149 > + RefPtr<SimpleFontData> placeholderFontCopyInLoadingState = SimpleFontData::create(placeholderFont->platformData(), true, true); > + return placeholderFontCopyInLoadingState; No need for this extra local variable. Just return it, I think. > Source/WebCore/platform/graphics/FontCache.cpp:150 > + return cache.get(); I keep forgetting this in code I write myself, but we don’t need the ".get()" here! > Source/WebCore/platform/graphics/FontCache.cpp:151 > +}; Stray semicolon here. > Source/WebCore/platform/graphics/FontCache.cpp:367 > + return cache.get(); Same .get() comment. > Source/WebCore/platform/graphics/FontCache.cpp:382 > +PassRefPtr<SimpleFontData> FontCache::fontForFamily(const FontDescription& fontDescription, const AtomicString& family, bool checkingAlternateName) Return type should be RefPtr<SimpleFontData>. If we are touching a function like this we should be changing the return value from PassRefPtr to RefPtr. We don’t need the Pass types at all for return values or arguments. For arguments it should be Ref<X>&& or RefPtr<X>&&, and for return values, Ref<X> or RefPtr<X>. > Source/WebCore/platform/graphics/FontCache.cpp:391 > +PassRefPtr<SimpleFontData> FontCache::fontDataForPlatformData(const FontPlatformData& platformData) Return type should be Ref<SimpleFontData>. > Source/WebCore/platform/graphics/FontCache.cpp:492 > +PassRefPtr<FontData> FontCache::fontForFamilyAtIndex(const FontDescription& description, int& familyIndex, FontSelector* fontSelector) Return type should be RefPtr<FontData>. > Source/WebCore/platform/graphics/FontGlyphs.h:51 > - ~FontGlyphs() { releaseFontData(); } > + ~FontGlyphs(); Why do we have this explicitly defined? Can’t we just let the compiler generate this? Or maybe that creates unwanted inlining? Or a need to include more headers here? If it doesn’t, then I suggest deleting this completely. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:427 > -PassRefPtr<SimpleFontData> FontCache::getLastResortFallbackFont(const FontDescription& fontDescription, ShouldRetain shouldRetain) > +PassRefPtr<SimpleFontData> FontCache::lastResortFallbackFont(const FontDescription& fontDescription) Return type should be RefPtr<SimpleFontData>.
Comment on attachment 244263 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=244263&action=review >> Source/WebCore/platform/graphics/FontGlyphs.h:51 >> + ~FontGlyphs(); > > Why do we have this explicitly defined? Can’t we just let the compiler generate this? Or maybe that creates unwanted inlining? Or a need to include more headers here? If it doesn’t, then I suggest deleting this completely. To make it possible clean up includes mostly though I didn't do any of it.
Created attachment 244278 [details] patch2
> I keep forgetting this in code I write myself, but we don’t need the > ".get()" here! Maybe there shouldn't be get().
> Why RefPtr instead of Ref? Also, normally I like to use auto for things like > this. I’m kind of surprised that this function returns a smart pointer at > all. Can it return a raw reference instead, or am I missing the point in > some major way in asking that? Yeah, made it Ref. From the lifetime management perspective I think it would better to head to direction where we don't rely on the "cache" keeping things alive and so shouldn't be handing out any raw pointers.
Created attachment 244287 [details] another
Created attachment 244289 [details] another
https://trac.webkit.org/r178133
(In reply to comment #10) > https://trac.webkit.org/r178133 It broke the Apple Windows build - https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/66379
https://trac.webkit.org/r178142 fixed windows