Bug 140246 – Remove the concept of "retained" font

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 244263 [details]
patch

[WebKit Commit Bot]

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.

[Darin Adler]

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>.

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 244278 [details]
patch2

[Antti Koivisto]

> I keep forgetting this in code I write myself, but we don’t need the
> ".get()" here!

Maybe there shouldn't be get().

[Antti Koivisto]

> 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.

[Antti Koivisto]

Created attachment 244287 [details]
another

[Antti Koivisto]

Created attachment 244289 [details]
another

[Antti Koivisto]

https://trac.webkit.org/r178133

[Csaba Osztrogonác]

(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

[Antti Koivisto]

https://trac.webkit.org/r178142 fixed windows