[Webkit-unassigned] [Bug 140246] Remove the concept of "retained" font

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=140246

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #244263|review?                     |review+
              Flags|                            |

--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 244263
  --> https://bugs.webkit.org/attachment.cgi?id=244263
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>.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150108/ffd1f97e/attachment-0002.html>