[webkit-reviews] review denied: [Bug 27265] [Qt] font cache reworking : [Attachment 32941] proposed patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Kelemen Balázs
<Kelemen.Balazs.3 at stud.u-szeged.hu>'s request for review:
Bug 27265: [Qt] font cache reworking
https://bugs.webkit.org/show_bug.cgi?id=27265

Attachment 32941: proposed patch
https://bugs.webkit.org/attachment.cgi?id=32941&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

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