[webkit-reviews] review denied: [Bug 66153] Abandoned Memory: Temporary CSS Fonts May Never Be Purged : [Attachment 103796] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Joseph Pecoraro <joepeck at webkit.org>'s request for
review:
Bug 66153: Abandoned Memory: Temporary CSS Fonts May Never Be Purged
https://bugs.webkit.org/show_bug.cgi?id=66153

Attachment 103796: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=103796&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=103796&action=review


> Source/WebCore/css/CSSFontFaceSource.cpp:191
> -	   SimpleFontData* tempData =
fontCache()->getCachedFontData(fontDescription, m_string);
> +	   SimpleFontData* tempData =
fontCache()->getCachedFontData(fontDescription, m_string, false,
FontCache::DoNotRetain);
>	   if (!tempData)
>	       tempData =
fontCache()->getLastResortFallbackFont(fontDescription);

This is not an ideal fix, since it still creates a cache entry. If we can’t
(why?) go back to what this code looked like before, i.e. just getting a
FontPlatformData from the cache and creating a SimpleFontData here, maybe we
should add a FontCache function that takes a fontDescription and a family name
(or not, given the FIXME) and returns a newly-created, non-cached
SimpleFontData.

> Source/WebCore/platform/graphics/FontCache.cpp:200
>	   result = createFontPlatformData(fontDescription, familyName);
> -	   gFontPlatformDataCache->set(key, result);
> +	   if (result)
> +	       gFontPlatformDataCache->set(key, result);
>	   foundResult = result;

This looks wrong. We want to cache the fact that there is no platform font for
the description and family name we were passed, so that we don’t have to call
createFontPlatformData() over and over.