Bug 140246

Summary: Remove the concept of "retained" font
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mmaxfield, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
darin: review+
patch2
none
another
none
another none

Description Antti Koivisto 2015-01-08 07:57:20 PST
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.
Comment 1 Antti Koivisto 2015-01-08 08:56:23 PST
Created attachment 244263 [details]
patch
Comment 2 WebKit Commit Bot 2015-01-08 09:01:00 PST
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 3 Darin Adler 2015-01-08 09:14:00 PST
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 4 Antti Koivisto 2015-01-08 09:31:06 PST
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.
Comment 5 Antti Koivisto 2015-01-08 11:52:36 PST
Created attachment 244278 [details]
patch2
Comment 6 Antti Koivisto 2015-01-08 11:59:17 PST
> I keep forgetting this in code I write myself, but we don’t need the
> ".get()" here!

Maybe there shouldn't be get().
Comment 7 Antti Koivisto 2015-01-08 12:11:15 PST
> 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.
Comment 8 Antti Koivisto 2015-01-08 13:05:23 PST
Created attachment 244287 [details]
another
Comment 9 Antti Koivisto 2015-01-08 13:23:28 PST
Created attachment 244289 [details]
another
Comment 10 Antti Koivisto 2015-01-08 14:30:41 PST
https://trac.webkit.org/r178133
Comment 11 Csaba Osztrogonác 2015-01-08 15:13:28 PST
(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
Comment 12 Antti Koivisto 2015-01-08 15:58:41 PST
https://trac.webkit.org/r178142 fixed windows