Bug 27265 – [Qt] font cache reworking

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

We have already done some work in #24551 but I am not fully happy with the code yet.
We created a cache for FontPlatformData objects in FontCacheQt.cpp. The problem is that
this cache never been purged. FontCache has a tricky semantic: the elements of
the FontPlatformDataCache are removed when a SimpleFontData, which has the same
FontPlatformData is being removed from the FontDataCache (the two FontPlatformData objects are
not the same, they are just equal). I think we should uniforming our FontCache implementation
with the shared one for adapting this semantic.

[Balazs Kelemen]

Created attachment 32777 [details]
proposed patch

[Balazs Kelemen]

I used WTF containers rather than Qt ones because as I know it is a policy in WebKit. The extremely low values for cTargeInactiveFontData and cMaxInactiveFontData are just for my testings - i forgot to change it for the patch.

[Simon Hausmann]

Comment on attachment 32777 [details]
proposed patch

In general this looks good, but after running the layout tests with your patch applied I'm getting a bunch of crashes:

fast/css/font-face-multiple-families.html
fast/loader/font-face-empty.html
fast/loader/goto-anchor-infinite-layout.html
svg/custom/createImageElement2.xhtml
svg/custom/font-platformDestroy-crash.svg
svg/dom/altGlyph-dom.xhtml

r- because of that, sorry :)

[Balazs Kelemen]

Created attachment 32941 [details]
proposed patch

Layouttest problems are fixed.

[Simon Hausmann]

Comment on attachment 32941 [details]
proposed patch


Most of the patch looks good to me! I have a few small comments below. r- because of those comments and a missing ChangeLog elaborating the changes.

One thing I've noticed is that the use of std::pair<const FontData*, bool> doesn't help very much to create readable code. Code like

    if (!item.second)
        return item.first;

is very very hard to read. This isn't your fault of course :-), but one thing that would be nice is a comment explaining what happens there, to make it easier to read the code three months later :-)

> -typedef QHash<FontDescription, FontPlatformData*> FontPlatformDataCache;
> +// This type must be consistent with FontPlatformData's ctor - the one which
> +// gets FontDescription as it's parameter.
> +class FontPlatformDataCacheKey {
> +public:
> +    FontPlatformDataCacheKey(const FontDescription& description)
> +        : m_familyName()
> +        , m_bold(false)
> +        , m_size(qRound(description.computedPixelSize()))

Why do you use qRound here? computedPixelSize() appears to return an int.


> +void FontCache::purgeInactiveFontData(int count)
> +{
> +    static bool isPurging;  // Guard against reentry when e.g. a deleted FontData releases its small caps FontData.
> +    if (isPurging)
> +        return;
> +
> +    isPurging = true;

From what I can see a chunk of this patch comes from FontCache.cpp. I believe your patch should include the addition of the copyright holders of that file to FontCacheQt.cpp.


> -    const FontData* result = new SimpleFontData(FontPlatformData(description), _font->wordSpacing(), _font->letterSpacing());
> -    m_fontList.append(pair<const FontData*, bool>(result, result->isCustomFont()));
> +    const FontData* result = new SimpleFontData(FontPlatformData(description, _font->wordSpacing(), _font->letterSpacing()), true);
> +    m_fontList.append(pair<const FontData*, bool>(result, true));

I may be confusing the code here (sorry if that's the case), but shouldn't this be "false" instead of "true"?

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 32941 [details])
> 
> Most of the patch looks good to me! I have a few small comments below. r-
> because of those comments and a missing ChangeLog elaborating the changes.
> 
> One thing I've noticed is that the use of std::pair<const FontData*, bool>
> doesn't help very much to create readable code. Code like
> 
>     if (!item.second)
>         return item.first;
> 
> is very very hard to read. This isn't your fault of course :-), but one thing
> that would be nice is a comment explaining what happens there, to make it
> easier to read the code three months later :-)

I will add some comments.

> 
> > -typedef QHash<FontDescription, FontPlatformData*> FontPlatformDataCache;
> > +// This type must be consistent with FontPlatformData's ctor - the one which
> > +// gets FontDescription as it's parameter.
> > +class FontPlatformDataCacheKey {
> > +public:
> > +    FontPlatformDataCacheKey(const FontDescription& description)
> > +        : m_familyName()
> > +        , m_bold(false)
> > +        , m_size(qRound(description.computedPixelSize()))
> 
> Why do you use qRound here? computedPixelSize() appears to return an int.
> 

I used it accidentally :) .

> 
> > +void FontCache::purgeInactiveFontData(int count)
> > +{
> > +    static bool isPurging;  // Guard against reentry when e.g. a deleted FontData releases its small caps FontData.
> > +    if (isPurging)
> > +        return;
> > +
> > +    isPurging = true;
> 
> From what I can see a chunk of this patch comes from FontCache.cpp. I believe
> your patch should include the addition of the copyright holders of that file to
> FontCacheQt.cpp.

Agree.

> 
> 
> > -    const FontData* result = new SimpleFontData(FontPlatformData(description), _font->wordSpacing(), _font->letterSpacing());
> > -    m_fontList.append(pair<const FontData*, bool>(result, result->isCustomFont()));
> > +    const FontData* result = new SimpleFontData(FontPlatformData(description, _font->wordSpacing(), _font->letterSpacing()), true);
> > +    m_fontList.append(pair<const FontData*, bool>(result, true));
> 
> I may be confusing the code here (sorry if that's the case), but shouldn't this
> be "false" instead of "true"?

No. I use the bool in the meaning of the object was created locally or not (when we got it from m_fontSelector). In the first case we can delete these in releaseFontData while in the latter we should call FontCache::releaseFontData. I do not exactly know what "custom" fonts means but this bools was very useful to me :)

[Balazs Kelemen]

Created attachment 33080 [details]
proposed patch

[Simon Hausmann]

Comment on attachment 33080 [details]
proposed patch

r=me. Noticed a small coding style glitch (missing space after if), but I'll fix it when landing. Thanks!

[Simon Hausmann]

Landed in r46124