[Webkit-unassigned] [Bug 27265] [Qt] font cache reworking

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Kelemen Balázs <Kelemen.Balazs.3 at stud.u-szeged.hu>  2009-07-20 05:42:39 PDT ---
(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 :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.