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

[bugzilla-daemon at webkit.org]

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


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32941|review?(hausmann at webkit.org |review-
               Flag|)                           |




--- Comment #5 from Simon Hausmann <hausmann at webkit.org>  2009-07-20 01:16:12 PDT ---
(From update of attachment 32941)

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"?

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